From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This issue has been fixed in the mainline. Here I have backported the relevant commits for the linux-6.6.y branch and attached them. With these patches, tpdir2 works as:
root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 TAP version 13 1..5 ok 0 skipped, TPIDR2 not supported ok 1 skipped, TPIDR2 not supported ok 2 skipped, TPIDR2 not supported ok 3 skipped, TPIDR2 not supported ok 4 skipped, TPIDR2 not supported
This issue is resolved by the first patch. However, to ensure functional completeness, all related patches were backported according to the following link.
https://lore.kernel.org/all/20230929031716.it.155-kees@kernel.org/#t
Eric W. Biederman (1): binfmt_elf: Support segments with 0 filesz and misaligned starts
Kees Cook (5): binfmt_elf: elf_bss no longer used by load_elf_binary() binfmt_elf: Use elf_load() for interpreter binfmt_elf: Use elf_load() for library binfmt_elf: Only report padzero() errors when PROT_WRITE mm: Remove unused vm_brk()
fs/binfmt_elf.c | 215 ++++++++++++++++----------------------------- include/linux/mm.h | 3 +- mm/mmap.c | 6 -- mm/nommu.c | 5 -- 4 files changed, 76 insertions(+), 153 deletions(-)
From: "Eric W. Biederman" ebiederm@xmission.com
commit 585a018627b4d7ed37387211f667916840b5c5ea upstream
Implement a helper elf_load() that wraps elf_map() and performs all of the necessary work to ensure that when "memsz > filesz" the bytes described by "memsz > filesz" are zeroed.
An outstanding issue is if the first segment has filesz 0, and has a randomized location. But that is the same as today.
In this change I replaced an open coded padzero() that did not clear all of the way to the end of the page, with padzero() that does.
I also stopped checking the return of padzero() as there is at least one known case where testing for failure is the wrong thing to do. It looks like binfmt_elf_fdpic may have the proper set of tests for when error handling can be safely completed.
I found a couple of commits in the old history https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git, that look very interesting in understanding this code.
commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail") commit c6e2227e4a3e ("[SPARC64]: Missing user access return value checks in fs/binfmt_elf.c and fs/compat.c") commit 5bf3be033f50 ("v2.4.10.1 -> v2.4.10.2")
Looking at commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail"):
commit 39b56d902bf35241e7cba6cc30b828ed937175ad Author: Pavel Machek pavel@ucw.cz Date: Wed Feb 9 22:40:30 2005 -0800
[PATCH] binfmt_elf: clearing bss may fail So we discover that Borland's Kylix application builder emits weird elf files which describe a non-writeable bss segment. So remove the clear_user() check at the place where we zero out the bss. I don't _think_ there are any security implications here (plus we've never checked that clear_user() return value, so whoops if it is a problem). Signed-off-by: Pavel Machek <pavel@suse.cz> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
It seems pretty clear that binfmt_elf_fdpic with skipping clear_user() for non-writable segments and otherwise calling clear_user(), aka padzero(), and checking it's return code is the right thing to do.
I just skipped the error checking as that avoids breaking things.
And notably, it looks like Borland's Kylix died in 2005 so it might be safe to just consider read-only segments with memsz > filesz an error.
Reported-by: Sebastian Ott sebott@redhat.com Reported-by: Thomas Weißschuh linux@weissschuh.net Closes: https://lkml.kernel.org/r/20230914-bss-alloc-v1-1-78de67d2c6dd@weissschuh.ne... Signed-off-by: "Eric W. Biederman" ebiederm@xmission.com Link: https://lore.kernel.org/r/87sf71f123.fsf@email.froward.int.ebiederm.org Tested-by: Pedro Falcato pedro.falcato@gmail.com Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-1-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Wenlin Kang wenlin.kang@windriver.com --- fs/binfmt_elf.c | 111 +++++++++++++++++++++--------------------------- 1 file changed, 48 insertions(+), 63 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index fb2c8d14327a..d59bca23c4bd 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -110,25 +110,6 @@ static struct linux_binfmt elf_format = {
#define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE))
-static int set_brk(unsigned long start, unsigned long end, int prot) -{ - start = ELF_PAGEALIGN(start); - end = ELF_PAGEALIGN(end); - if (end > start) { - /* - * Map the last of the bss segment. - * If the header is requesting these pages to be - * executable, honour that (ppc32 needs this). - */ - int error = vm_brk_flags(start, end - start, - prot & PROT_EXEC ? VM_EXEC : 0); - if (error) - return error; - } - current->mm->start_brk = current->mm->brk = end; - return 0; -} - /* We need to explicitly zero any fractional pages after the data section (i.e. bss). This would contain the junk from the file that should not @@ -406,6 +387,51 @@ static unsigned long elf_map(struct file *filep, unsigned long addr, return(map_addr); }
+static unsigned long elf_load(struct file *filep, unsigned long addr, + const struct elf_phdr *eppnt, int prot, int type, + unsigned long total_size) +{ + unsigned long zero_start, zero_end; + unsigned long map_addr; + + if (eppnt->p_filesz) { + map_addr = elf_map(filep, addr, eppnt, prot, type, total_size); + if (BAD_ADDR(map_addr)) + return map_addr; + if (eppnt->p_memsz > eppnt->p_filesz) { + zero_start = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) + + eppnt->p_filesz; + zero_end = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) + + eppnt->p_memsz; + + /* Zero the end of the last mapped page */ + padzero(zero_start); + } + } else { + map_addr = zero_start = ELF_PAGESTART(addr); + zero_end = zero_start + ELF_PAGEOFFSET(eppnt->p_vaddr) + + eppnt->p_memsz; + } + if (eppnt->p_memsz > eppnt->p_filesz) { + /* + * Map the last of the segment. + * If the header is requesting these pages to be + * executable, honour that (ppc32 needs this). + */ + int error; + + zero_start = ELF_PAGEALIGN(zero_start); + zero_end = ELF_PAGEALIGN(zero_end); + + error = vm_brk_flags(zero_start, zero_end - zero_start, + prot & PROT_EXEC ? VM_EXEC : 0); + if (error) + map_addr = error; + } + return map_addr; +} + + static unsigned long total_mapping_size(const struct elf_phdr *phdr, int nr) { elf_addr_t min_addr = -1; @@ -829,7 +855,6 @@ static int load_elf_binary(struct linux_binprm *bprm) struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL; struct elf_phdr *elf_property_phdata = NULL; unsigned long elf_bss, elf_brk; - int bss_prot = 0; int retval, i; unsigned long elf_entry; unsigned long e_entry; @@ -1041,33 +1066,6 @@ static int load_elf_binary(struct linux_binprm *bprm) if (elf_ppnt->p_type != PT_LOAD) continue;
- if (unlikely (elf_brk > elf_bss)) { - unsigned long nbyte; - - /* There was a PT_LOAD segment with p_memsz > p_filesz - before this one. Map anonymous pages, if needed, - and clear the area. */ - retval = set_brk(elf_bss + load_bias, - elf_brk + load_bias, - bss_prot); - if (retval) - goto out_free_dentry; - nbyte = ELF_PAGEOFFSET(elf_bss); - if (nbyte) { - nbyte = ELF_MIN_ALIGN - nbyte; - if (nbyte > elf_brk - elf_bss) - nbyte = elf_brk - elf_bss; - if (clear_user((void __user *)elf_bss + - load_bias, nbyte)) { - /* - * This bss-zeroing can fail if the ELF - * file specifies odd protections. So - * we don't check the return value - */ - } - } - } - elf_prot = make_prot(elf_ppnt->p_flags, &arch_state, !!interpreter, false);
@@ -1163,7 +1161,7 @@ static int load_elf_binary(struct linux_binprm *bprm) } }
- error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt, + error = elf_load(bprm->file, load_bias + vaddr, elf_ppnt, elf_prot, elf_flags, total_size); if (BAD_ADDR(error)) { retval = IS_ERR_VALUE(error) ? @@ -1218,10 +1216,8 @@ static int load_elf_binary(struct linux_binprm *bprm) if (end_data < k) end_data = k; k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz; - if (k > elf_brk) { - bss_prot = elf_prot; + if (k > elf_brk) elf_brk = k; - } }
e_entry = elf_ex->e_entry + load_bias; @@ -1233,18 +1229,7 @@ static int load_elf_binary(struct linux_binprm *bprm) start_data += load_bias; end_data += load_bias;
- /* Calling set_brk effectively mmaps the pages that we need - * for the bss and break sections. We must do this before - * mapping in the interpreter, to make sure it doesn't wind - * up getting placed where the bss needs to go. - */ - retval = set_brk(elf_bss, elf_brk, bss_prot); - if (retval) - goto out_free_dentry; - if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bss))) { - retval = -EFAULT; /* Nobody gets to see this, but.. */ - goto out_free_dentry; - } + current->mm->start_brk = current->mm->brk = ELF_PAGEALIGN(elf_brk);
if (interpreter) { elf_entry = load_elf_interp(interp_elf_ex,
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: 585a018627b4d7ed37387211f667916840b5c5ea
WARNING: Author mismatch between patch and upstream commit: Backport author: Kang Wenlinwenlin.kang@windriver.com Commit author: Eric W. Biedermanebiederm@xmission.com
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- 1: 585a018627b4d ! 1: bb3401d71e115 binfmt_elf: Support segments with 0 filesz and misaligned starts @@ Metadata ## Commit message ## binfmt_elf: Support segments with 0 filesz and misaligned starts
+ commit 585a018627b4d7ed37387211f667916840b5c5ea upstream + Implement a helper elf_load() that wraps elf_map() and performs all of the necessary work to ensure that when "memsz > filesz" the bytes described by "memsz > filesz" are zeroed. @@ Commit message Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-1-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org + Signed-off-by: Wenlin Kang wenlin.kang@windriver.com
## fs/binfmt_elf.c ## @@ fs/binfmt_elf.c: static struct linux_binfmt elf_format = { ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.6.y | Success | Success |
From: Kees Cook keescook@chromium.org
commit 8ed2ef21ff564cf4a25c098ace510ee6513c9836 upstream
With the BSS handled generically via the new filesz/memsz mismatch handling logic in elf_load(), elf_bss no longer needs to be tracked. Drop the variable.
Cc: Eric Biederman ebiederm@xmission.com Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman ebiederm@xmission.com Tested-by: Pedro Falcato pedro.falcato@gmail.com Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-2-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Wenlin Kang wenlin.kang@windriver.com --- fs/binfmt_elf.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index d59bca23c4bd..02258b28e370 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -854,7 +854,7 @@ static int load_elf_binary(struct linux_binprm *bprm) unsigned long error; struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL; struct elf_phdr *elf_property_phdata = NULL; - unsigned long elf_bss, elf_brk; + unsigned long elf_brk; int retval, i; unsigned long elf_entry; unsigned long e_entry; @@ -1046,7 +1046,6 @@ static int load_elf_binary(struct linux_binprm *bprm) if (retval < 0) goto out_free_dentry;
- elf_bss = 0; elf_brk = 0;
start_code = ~0UL; @@ -1209,8 +1208,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
- if (k > elf_bss) - elf_bss = k; if ((elf_ppnt->p_flags & PF_X) && end_code < k) end_code = k; if (end_data < k) @@ -1222,7 +1219,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
e_entry = elf_ex->e_entry + load_bias; phdr_addr += load_bias; - elf_bss += load_bias; elf_brk += load_bias; start_code += load_bias; end_code += load_bias;
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: 8ed2ef21ff564cf4a25c098ace510ee6513c9836
WARNING: Author mismatch between patch and upstream commit: Backport author: Kang Wenlinwenlin.kang@windriver.com Commit author: Kees Cookkeescook@chromium.org
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- 1: 8ed2ef21ff564 ! 1: f18629afe7b56 binfmt_elf: elf_bss no longer used by load_elf_binary() @@ Metadata ## Commit message ## binfmt_elf: elf_bss no longer used by load_elf_binary()
+ commit 8ed2ef21ff564cf4a25c098ace510ee6513c9836 upstream + With the BSS handled generically via the new filesz/memsz mismatch handling logic in elf_load(), elf_bss no longer needs to be tracked. Drop the variable. @@ Commit message Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-2-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org + Signed-off-by: Wenlin Kang wenlin.kang@windriver.com
## fs/binfmt_elf.c ## @@ fs/binfmt_elf.c: static int load_elf_binary(struct linux_binprm *bprm) ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.12.y | Success | Success |
From: Kees Cook keescook@chromium.org
commit 8b04d32678e3c46b8a738178e0e55918eaa3be17 upstream
Handle arbitrary memsz>filesz in interpreter ELF segments, instead of only supporting it in the last segment (which is expected to be the BSS).
Cc: Eric Biederman ebiederm@xmission.com Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Reported-by: Pedro Falcato pedro.falcato@gmail.com Closes: https://lore.kernel.org/lkml/20221106021657.1145519-1-pedro.falcato@gmail.co... Tested-by: Pedro Falcato pedro.falcato@gmail.com Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-3-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Wenlin Kang wenlin.kang@windriver.com --- fs/binfmt_elf.c | 46 +--------------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 02258b28e370..bed3c0cfb63f 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -622,8 +622,6 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, struct elf_phdr *eppnt; unsigned long load_addr = 0; int load_addr_set = 0; - unsigned long last_bss = 0, elf_bss = 0; - int bss_prot = 0; unsigned long error = ~0UL; unsigned long total_size; int i; @@ -660,7 +658,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, else if (no_base && interp_elf_ex->e_type == ET_DYN) load_addr = -vaddr;
- map_addr = elf_map(interpreter, load_addr + vaddr, + map_addr = elf_load(interpreter, load_addr + vaddr, eppnt, elf_prot, elf_type, total_size); total_size = 0; error = map_addr; @@ -686,51 +684,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, error = -ENOMEM; goto out; } - - /* - * Find the end of the file mapping for this phdr, and - * keep track of the largest address we see for this. - */ - k = load_addr + eppnt->p_vaddr + eppnt->p_filesz; - if (k > elf_bss) - elf_bss = k; - - /* - * Do the same thing for the memory mapping - between - * elf_bss and last_bss is the bss section. - */ - k = load_addr + eppnt->p_vaddr + eppnt->p_memsz; - if (k > last_bss) { - last_bss = k; - bss_prot = elf_prot; - } } }
- /* - * Now fill out the bss section: first pad the last page from - * the file up to the page boundary, and zero it from elf_bss - * up to the end of the page. - */ - if (padzero(elf_bss)) { - error = -EFAULT; - goto out; - } - /* - * Next, align both the file and mem bss up to the page size, - * since this is where elf_bss was just zeroed up to, and where - * last_bss will end after the vm_brk_flags() below. - */ - elf_bss = ELF_PAGEALIGN(elf_bss); - last_bss = ELF_PAGEALIGN(last_bss); - /* Finally, if there is still more bss to allocate, do it. */ - if (last_bss > elf_bss) { - error = vm_brk_flags(elf_bss, last_bss - elf_bss, - bss_prot & PROT_EXEC ? VM_EXEC : 0); - if (error) - goto out; - } - error = load_addr; out: return error;
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: 8b04d32678e3c46b8a738178e0e55918eaa3be17
WARNING: Author mismatch between patch and upstream commit: Backport author: Kang Wenlinwenlin.kang@windriver.com Commit author: Kees Cookkeescook@chromium.org
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- 1: 8b04d32678e3c ! 1: 498e3c72603ad binfmt_elf: Use elf_load() for interpreter @@ Metadata ## Commit message ## binfmt_elf: Use elf_load() for interpreter
+ commit 8b04d32678e3c46b8a738178e0e55918eaa3be17 upstream + Handle arbitrary memsz>filesz in interpreter ELF segments, instead of only supporting it in the last segment (which is expected to be the BSS). @@ Commit message Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-3-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org + Signed-off-by: Wenlin Kang wenlin.kang@windriver.com
## fs/binfmt_elf.c ## @@ fs/binfmt_elf.c: static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.12.y | Success | Success |
From: Kees Cook keescook@chromium.org
commit d5ca24f639588811af57ceac513183fa2004bd3a upstream
While load_elf_library() is a libc5-ism, we can still replace most of its contents with elf_load() as well, further simplifying the code.
Some historical context: - libc4 was a.out and used uselib (a.out support has been removed) - libc5 was ELF and used uselib (there may still be users) - libc6 is ELF and has never used uselib
Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman ebiederm@xmission.com Tested-by: Pedro Falcato pedro.falcato@gmail.com Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-4-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Wenlin Kang wenlin.kang@windriver.com --- fs/binfmt_elf.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index bed3c0cfb63f..a6508c56f418 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1307,7 +1307,6 @@ static int load_elf_library(struct file *file) { struct elf_phdr *elf_phdata; struct elf_phdr *eppnt; - unsigned long elf_bss, bss, len; int retval, error, i, j; struct elfhdr elf_ex;
@@ -1352,30 +1351,15 @@ static int load_elf_library(struct file *file) eppnt++;
/* Now use mmap to map the library into memory. */ - error = vm_mmap(file, - ELF_PAGESTART(eppnt->p_vaddr), - (eppnt->p_filesz + - ELF_PAGEOFFSET(eppnt->p_vaddr)), + error = elf_load(file, ELF_PAGESTART(eppnt->p_vaddr), + eppnt, PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED_NOREPLACE | MAP_PRIVATE, - (eppnt->p_offset - - ELF_PAGEOFFSET(eppnt->p_vaddr))); - if (error != ELF_PAGESTART(eppnt->p_vaddr)) - goto out_free_ph; + 0);
- elf_bss = eppnt->p_vaddr + eppnt->p_filesz; - if (padzero(elf_bss)) { - error = -EFAULT; + if (error != ELF_PAGESTART(eppnt->p_vaddr)) goto out_free_ph; - }
- len = ELF_PAGEALIGN(eppnt->p_filesz + eppnt->p_vaddr); - bss = ELF_PAGEALIGN(eppnt->p_memsz + eppnt->p_vaddr); - if (bss > len) { - error = vm_brk(len, bss - len); - if (error) - goto out_free_ph; - } error = 0;
out_free_ph:
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: d5ca24f639588811af57ceac513183fa2004bd3a
WARNING: Author mismatch between patch and upstream commit: Backport author: Kang Wenlinwenlin.kang@windriver.com Commit author: Kees Cookkeescook@chromium.org
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- 1: d5ca24f639588 ! 1: 7498fc234c462 binfmt_elf: Use elf_load() for library @@ Metadata ## Commit message ## binfmt_elf: Use elf_load() for library
+ commit d5ca24f639588811af57ceac513183fa2004bd3a upstream + While load_elf_library() is a libc5-ism, we can still replace most of its contents with elf_load() as well, further simplifying the code.
@@ Commit message Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-4-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org + Signed-off-by: Wenlin Kang wenlin.kang@windriver.com
## fs/binfmt_elf.c ## @@ fs/binfmt_elf.c: static int load_elf_library(struct file *file) ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.12.y | Success | Success |
From: Kees Cook keescook@chromium.org
commit f9c0a39d95301a36baacfd3495374c6128d662fa upstream
Errors with padzero() should be caught unless we're expecting a pathological (non-writable) segment. Report -EFAULT only when PROT_WRITE is present.
Additionally add some more documentation to padzero(), elf_map(), and elf_load().
Cc: Eric Biederman ebiederm@xmission.com Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman ebiederm@xmission.com Tested-by: Pedro Falcato pedro.falcato@gmail.com Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-5-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Wenlin Kang wenlin.kang@windriver.com --- fs/binfmt_elf.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index a6508c56f418..09f9c5ad0fc1 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -110,19 +110,19 @@ static struct linux_binfmt elf_format = {
#define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE))
-/* We need to explicitly zero any fractional pages - after the data section (i.e. bss). This would - contain the junk from the file that should not - be in memory +/* + * We need to explicitly zero any trailing portion of the page that follows + * p_filesz when it ends before the page ends (e.g. bss), otherwise this + * memory will contain the junk from the file that should not be present. */ -static int padzero(unsigned long elf_bss) +static int padzero(unsigned long address) { unsigned long nbyte;
- nbyte = ELF_PAGEOFFSET(elf_bss); + nbyte = ELF_PAGEOFFSET(address); if (nbyte) { nbyte = ELF_MIN_ALIGN - nbyte; - if (clear_user((void __user *) elf_bss, nbyte)) + if (clear_user((void __user *)address, nbyte)) return -EFAULT; } return 0; @@ -348,6 +348,11 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, return 0; }
+/* + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset" + * into memory at "addr". (Note that p_filesz is rounded up to the + * next page, so any extra bytes from the file must be wiped.) + */ static unsigned long elf_map(struct file *filep, unsigned long addr, const struct elf_phdr *eppnt, int prot, int type, unsigned long total_size) @@ -387,6 +392,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr, return(map_addr); }
+/* + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset" + * into memory at "addr". Memory from "p_filesz" through "p_memsz" + * rounded up to the next page is zeroed. + */ static unsigned long elf_load(struct file *filep, unsigned long addr, const struct elf_phdr *eppnt, int prot, int type, unsigned long total_size) @@ -404,8 +414,12 @@ static unsigned long elf_load(struct file *filep, unsigned long addr, zero_end = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) + eppnt->p_memsz;
- /* Zero the end of the last mapped page */ - padzero(zero_start); + /* + * Zero the end of the last mapped page but ignore + * any errors if the segment isn't writable. + */ + if (padzero(zero_start) && (prot & PROT_WRITE)) + return -EFAULT; } } else { map_addr = zero_start = ELF_PAGESTART(addr);
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: f9c0a39d95301a36baacfd3495374c6128d662fa
WARNING: Author mismatch between patch and upstream commit: Backport author: Kang Wenlinwenlin.kang@windriver.com Commit author: Kees Cookkeescook@chromium.org
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- 1: f9c0a39d95301 ! 1: d7251c2864622 binfmt_elf: Only report padzero() errors when PROT_WRITE @@ Metadata ## Commit message ## binfmt_elf: Only report padzero() errors when PROT_WRITE
+ commit f9c0a39d95301a36baacfd3495374c6128d662fa upstream + Errors with padzero() should be caught unless we're expecting a pathological (non-writable) segment. Report -EFAULT only when PROT_WRITE is present. @@ Commit message Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-5-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org + Signed-off-by: Wenlin Kang wenlin.kang@windriver.com
## fs/binfmt_elf.c ## @@ fs/binfmt_elf.c: static struct linux_binfmt elf_format = { ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.12.y | Success | Success |
From: Kees Cook keescook@chromium.org
commit 2632bb84d1d53cfd6cf65261064273ded4f759d5 upstream
With fs/binfmt_elf.c fully refactored to use the new elf_load() helper, there are no more users of vm_brk(), so remove it.
Cc: Andrew Morton akpm@linux-foundation.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman ebiederm@xmission.com Tested-by: Pedro Falcato pedro.falcato@gmail.com Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-6-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Wenlin Kang wenlin.kang@windriver.com --- include/linux/mm.h | 3 +-- mm/mmap.c | 6 ------ mm/nommu.c | 5 ----- 3 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h index 209370f64436..586e8d216be8 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3315,8 +3315,7 @@ static inline void mm_populate(unsigned long addr, unsigned long len) static inline void mm_populate(unsigned long addr, unsigned long len) {} #endif
-/* These take the mm semaphore themselves */ -extern int __must_check vm_brk(unsigned long, unsigned long); +/* This takes the mm semaphore itself */ extern int __must_check vm_brk_flags(unsigned long, unsigned long, unsigned long); extern int vm_munmap(unsigned long, size_t); extern unsigned long __must_check vm_mmap(struct file *, unsigned long, diff --git a/mm/mmap.c b/mm/mmap.c index 03a24cb3951d..c43048fc493e 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -3256,12 +3256,6 @@ int vm_brk_flags(unsigned long addr, unsigned long request, unsigned long flags) } EXPORT_SYMBOL(vm_brk_flags);
-int vm_brk(unsigned long addr, unsigned long len) -{ - return vm_brk_flags(addr, len, 0); -} -EXPORT_SYMBOL(vm_brk); - /* Release all mmaps. */ void exit_mmap(struct mm_struct *mm) { diff --git a/mm/nommu.c b/mm/nommu.c index 3228b2d3e4ab..346b26b2660c 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -1537,11 +1537,6 @@ void exit_mmap(struct mm_struct *mm) mmap_write_unlock(mm); }
-int vm_brk(unsigned long addr, unsigned long len) -{ - return -ENOMEM; -} - /* * expand (or shrink) an existing mapping, potentially moving it at the same * time (controlled by the MREMAP_MAYMOVE flag and available VM space)
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: 2632bb84d1d53cfd6cf65261064273ded4f759d5
WARNING: Author mismatch between patch and upstream commit: Backport author: Kang Wenlinwenlin.kang@windriver.com Commit author: Kees Cookkeescook@chromium.org
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- 1: 2632bb84d1d53 ! 1: 3f390c20a6af6 mm: Remove unused vm_brk() @@ Metadata ## Commit message ## mm: Remove unused vm_brk()
+ commit 2632bb84d1d53cfd6cf65261064273ded4f759d5 upstream + With fs/binfmt_elf.c fully refactored to use the new elf_load() helper, there are no more users of vm_brk(), so remove it.
@@ Commit message Signed-off-by: Sebastian Ott sebott@redhat.com Link: https://lore.kernel.org/r/20230929032435.2391507-6-keescook@chromium.org Signed-off-by: Kees Cook keescook@chromium.org + Signed-off-by: Wenlin Kang wenlin.kang@windriver.com
## include/linux/mm.h ## @@ include/linux/mm.h: static inline void mm_populate(unsigned long addr, unsigned long len) ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.12.y | Success | Success |
On Wed, Apr 02, 2025 at 04:26:50PM +0800, Kang Wenlin wrote:
From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This is just a userspace issue (i.e. don't do that, and if you do want to do that, use a new kernel!)
Why do these changes need to be backported, do you have real users that are crashing in this way to require these changes?
thanks,
greg k-h
Hi Greg
Thanks for your response.
On 4/3/2025 22:52, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Wed, Apr 02, 2025 at 04:26:50PM +0800, Kang Wenlin wrote:
From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This is just a userspace issue (i.e. don't do that, and if you do want to do that, use a new kernel!)
Why do these changes need to be backported, do you have real users that are crashing in this way to require these changes?
This issue was identified during our internal testing, and I found similar cases discussed in the link above. Upon reviewing the kernel code, I noticed that a patch series already accepted into mainline addresses this problem. Since these patches are already upstream and effectively resolve the issue, I decided to backport them. We believe this provides a more robust and maintainable solution compared to relying on users to avoid the triggering behavior.
thanks,
greg k-h
On Fri, Apr 04, 2025 at 03:58:36PM +0800, Kang Wenlin wrote:
Hi Greg
Thanks for your response.
On 4/3/2025 22:52, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Wed, Apr 02, 2025 at 04:26:50PM +0800, Kang Wenlin wrote:
From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This is just a userspace issue (i.e. don't do that, and if you do want to do that, use a new kernel!)
Why do these changes need to be backported, do you have real users that are crashing in this way to require these changes?
This issue was identified during our internal testing, and I found similar cases discussed in the link above. Upon reviewing the kernel code, I noticed that a patch series already accepted into mainline addresses this problem. Since these patches are already upstream and effectively resolve the issue, I decided to backport them. We believe this provides a more robust and maintainable solution compared to relying on users to avoid the triggering behavior.
Fixing something just to get the selftests to pass is fine, but do you actually know of a real-world case where this is a problem that needs to be resolved? That's what I'm asking here, do you have users that have run into this issue? I ask as it's not a regression from what I can determine, but rather a new "feature".
thanks,
greg k-h
On 4/8/25 17:06, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Fri, Apr 04, 2025 at 03:58:36PM +0800, Kang Wenlin wrote:
Hi Greg
Thanks for your response.
On 4/3/2025 22:52, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Wed, Apr 02, 2025 at 04:26:50PM +0800, Kang Wenlin wrote:
From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This is just a userspace issue (i.e. don't do that, and if you do want to do that, use a new kernel!)
Why do these changes need to be backported, do you have real users that are crashing in this way to require these changes?
This issue was identified during our internal testing, and I found similar cases discussed in the link above. Upon reviewing the kernel code, I noticed that a patch series already accepted into mainline addresses this problem. Since these patches are already upstream and effectively resolve the issue, I decided to backport them. We believe this provides a more robust and maintainable solution compared to relying on users to avoid the triggering behavior.
Fixing something just to get the selftests to pass is fine, but do you actually know of a real-world case where this is a problem that needs to be resolved? That's what I'm asking here, do you have users that have run into this issue? I ask as it's not a regression from what I can determine, but rather a new "feature".
Thanks for your explanation. I’m not aware of any real-world cases. As of now, apart from our internal testing, we haven’t had any users report this issue.
thanks,
greg k-h
On Tue, Apr 08, 2025 at 05:50:06PM +0800, Wenlin Kang wrote:
On 4/8/25 17:06, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Fri, Apr 04, 2025 at 03:58:36PM +0800, Kang Wenlin wrote:
Hi Greg
Thanks for your response.
On 4/3/2025 22:52, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Wed, Apr 02, 2025 at 04:26:50PM +0800, Kang Wenlin wrote:
From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This is just a userspace issue (i.e. don't do that, and if you do want to do that, use a new kernel!)
Why do these changes need to be backported, do you have real users that are crashing in this way to require these changes?
This issue was identified during our internal testing, and I found similar cases discussed in the link above. Upon reviewing the kernel code, I noticed that a patch series already accepted into mainline addresses this problem. Since these patches are already upstream and effectively resolve the issue, I decided to backport them. We believe this provides a more robust and maintainable solution compared to relying on users to avoid the triggering behavior.
Fixing something just to get the selftests to pass is fine, but do you actually know of a real-world case where this is a problem that needs to be resolved? That's what I'm asking here, do you have users that have run into this issue? I ask as it's not a regression from what I can determine, but rather a new "feature".
Thanks for your explanation. I’m not aware of any real-world cases. As of now, apart from our internal testing, we haven’t had any users report this issue.
Ok, I'll drop this from the review queue.
thanks,
greg k-h
On 4/8/2025 17:55, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Apr 08, 2025 at 05:50:06PM +0800, Wenlin Kang wrote:
On 4/8/25 17:06, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Fri, Apr 04, 2025 at 03:58:36PM +0800, Kang Wenlin wrote:
Hi Greg
Thanks for your response.
On 4/3/2025 22:52, Greg KH wrote:
CAUTION: This email comes from a non Wind River email account! Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Wed, Apr 02, 2025 at 04:26:50PM +0800, Kang Wenlin wrote:
From: Wenlin Kang wenlin.kang@windriver.com
The selftest tpdir2 terminated with a 'Segmentation fault' during loading.
root@localhost:~# cd linux-kenel/tools/testing/selftests/arm64/abi && make root@localhost:~/linux-kernel/tools/testing/selftests/arm64/abi# ./tpidr2 Segmentation fault
The cause of this is the __arch_clear_user() failure.
load_elf_binary() [fs/binfmt_elf.c] -> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bes))) -> padzero() -> clear_user() [arch/arm64/include/asm/uaccess.h] -> __arch_clear_user() [arch/arm64/lib/clear_user.S]
For more details, please see: https://lore.kernel.org/lkml/1d0342f3-0474-482b-b6db-81ca7820a462@t-8ch.de/T...
This is just a userspace issue (i.e. don't do that, and if you do want to do that, use a new kernel!)
Why do these changes need to be backported, do you have real users that are crashing in this way to require these changes?
This issue was identified during our internal testing, and I found similar cases discussed in the link above. Upon reviewing the kernel code, I noticed that a patch series already accepted into mainline addresses this problem. Since these patches are already upstream and effectively resolve the issue, I decided to backport them. We believe this provides a more robust and maintainable solution compared to relying on users to avoid the triggering behavior.
Fixing something just to get the selftests to pass is fine, but do you actually know of a real-world case where this is a problem that needs to be resolved? That's what I'm asking here, do you have users that have run into this issue? I ask as it's not a regression from what I can determine, but rather a new "feature".
Thanks for your explanation. I’m not aware of any real-world cases. As of now, apart from our internal testing, we haven’t had any users report this issue.
Ok, I'll drop this from the review queue.
Got it, thanks for letting me know.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org