[PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Kang Wenlin]

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(-)

-- 
2.43.0

[PATCH 6.6.y 1/6] binfmt_elf: Support segments with 0 filesz and misaligned starts

[Kang Wenlin]

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.net
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,
-- 
2.43.0

Re: [PATCH 6.6.y 1/6] binfmt_elf: Support segments with 0 filesz and misaligned starts

[Sasha Levin]

[ 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 Wenlin<wenlin.kang@windriver.com>
Commit author: Eric W. Biederman<ebiederm@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   |

[PATCH 6.6.y 2/6] binfmt_elf: elf_bss no longer used by load_elf_binary()

[Kang Wenlin]

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;
-- 
2.43.0

Re: [PATCH 6.6.y 2/6] binfmt_elf: elf_bss no longer used by load_elf_binary()

[Sasha Levin]

[ 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 Wenlin<wenlin.kang@windriver.com>
Commit author: Kees Cook<keescook@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   |

[PATCH 6.6.y 3/6] binfmt_elf: Use elf_load() for interpreter

[Kang Wenlin]

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.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-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;
-- 
2.43.0

Re: [PATCH 6.6.y 3/6] binfmt_elf: Use elf_load() for interpreter

[Sasha Levin]

[ 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 Wenlin<wenlin.kang@windriver.com>
Commit author: Kees Cook<keescook@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   |

[PATCH 6.6.y 4/6] binfmt_elf: Use elf_load() for library

[Kang Wenlin]

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:
-- 
2.43.0

Re: [PATCH 6.6.y 4/6] binfmt_elf: Use elf_load() for library

[Sasha Levin]

[ 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 Wenlin<wenlin.kang@windriver.com>
Commit author: Kees Cook<keescook@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   |

[PATCH 6.6.y 5/6] binfmt_elf: Only report padzero() errors when PROT_WRITE

[Kang Wenlin]

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);
-- 
2.43.0

Re: [PATCH 6.6.y 5/6] binfmt_elf: Only report padzero() errors when PROT_WRITE

[Sasha Levin]

[ 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 Wenlin<wenlin.kang@windriver.com>
Commit author: Kees Cook<keescook@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   |

[PATCH 6.6.y 6/6] mm: Remove unused vm_brk()

[Kang Wenlin]

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)
-- 
2.43.0

Re: [PATCH 6.6.y 6/6] mm: Remove unused vm_brk()

[Sasha Levin]

[ 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 Wenlin<wenlin.kang@windriver.com>
Commit author: Kees Cook<keescook@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   |

Re: [PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Greg KH]

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

Re: [PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Kang Wenlin]

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

-- 
--
Thanks
Wenlin Kang

Re: [PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Greg KH]

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

Re: [PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Wenlin Kang]

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

Re: [PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Greg KH]

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

Re: [PATCH 6.6.y 0/6] Backported patches to fix selftest tpdir2

[Kang Wenlin]

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

-- 
--
Thanks
Wenlin Kang