exec: fix linux_binprm::exec in transfer_args_to_stack()

PATCH Applied Max Filippov 3 participants 8 messages Mar 20, 2024 – Mar 22, 2024

Subsystems (4)

Stable branch Memory management VFS exec and binfmt

lore

Share & tools

lore

Max Filippov

author

2024-03-20 18:26 UTC #0 lore raw

PATCH 1/1 exec: fix linux_binprm::exec in transfer_args_to_stack()

In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.

Adjust linux_binprm::exec before the successful return from the transfer_args_to_stack().

Cc [email protected]

Signed-off-by Max Filippov <[email protected]>

---
 fs/exec.c | 1 +
 1 file changed, 1 insertion(+)

fs/exec.c +1 -0

index af4fbb61cd53..5ee2545c3e18 100644
@@ -895,6 +895,7 @@ int transfer_args_to_stack(struct linux_binprm *bprm,
 			goto out;
 	}
 
+	bprm->exec += *sp_location - MAX_ARG_PAGES * PAGE_SIZE;
 	*sp_location = sp;
 
 out:

Signature · 1 line

2.39.2

Matthew Wilcox

2024-03-20 19:29 UTC #1 lore raw

Quoted 11 lines · Max Filippov

In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.
Adjust linux_binprm::exec before the successful return from the transfer_args_to_stack().

Do you know which commit broke this, ie how far back should this be backported? Or has it always been broken?

Quoted 22 lines

Cc [email protected]

Signed-off-by Max Filippov <[email protected]>

---
 fs/exec.c | 1 +
 1 file changed, 1 insertion(+)

fs/exec.c +1 -0

index af4fbb61cd53..5ee2545c3e18 100644
@@ -895,6 +895,7 @@ int transfer_args_to_stack(struct linux_binprm *bprm,
 			goto out;
 	}
 
+	bprm->exec += *sp_location - MAX_ARG_PAGES * PAGE_SIZE;
 	*sp_location = sp;
 
 out:

Signature · 1 line

2.39.2

Max Filippov

author

2024-03-20 20:31 UTC #2 lore raw

Quoted 11 lines · Matthew Wilcox

Quoted 9 lines · Max Filippov
In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.

Quoted 5 lines

Quoted 2 lines
Adjust linux_binprm::exec before the successful return from the transfer_args_to_stack().
Do you know which commit broke this, ie how far back should this be backported? Or has it always been broken?

From reading the code I see that linux_binprm::p started being an offset in the commit b6a2fea39318 ("mm: variable length argument support") which is v2.6.22-3328-gb6a2fea39318 and filling in the AT_EXECFN aux entry was added in the commit
5edc2a5123a7 ("binfmt_elf_fdpic: wire up AT_EXECFD, AT_EXECFN, AT_SECURE")
which is v2.6.27-4641-g5edc2a5123a7. I don't see any translation of the linux_binprm::exec at that time so to me it looks like it's always been broken.

--
Thanks.
-- Max

Kees Cook

2024-03-21 17:05 UTC #3 lore raw

In reply to Max Filippov

Quoted 11 lines · Max Filippov

In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.
Adjust linux_binprm::exec before the successful return from the transfer_args_to_stack().

What's the best way to test this? (Is there a qemu setup I can use to see the before/after of AT_EXECFN?)

How did you encounter the problem?

Quoted 22 lines

Cc [email protected]

Signed-off-by Max Filippov <[email protected]>

---
 fs/exec.c | 1 +
 1 file changed, 1 insertion(+)

fs/exec.c +1 -0

index af4fbb61cd53..5ee2545c3e18 100644
@@ -895,6 +895,7 @@ int transfer_args_to_stack(struct linux_binprm *bprm,
 			goto out;
 	}
 
+	bprm->exec += *sp_location - MAX_ARG_PAGES * PAGE_SIZE;
 	*sp_location = sp;
 
 out:

Signature · 1 line

2.39.2

Signature · 1 line

Kees Cook

Max Filippov

author

2024-03-21 19:52 UTC #4 lore raw

Quoted 11 lines · Kees Cook

Quoted 9 lines · Max Filippov
In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.

Quoted 5 lines

Quoted 2 lines
Adjust linux_binprm::exec before the successful return from the transfer_args_to_stack().
What's the best way to test this? (Is there a qemu setup I can use to see the before/after of AT_EXECFN?)

I put a readme with the steps to build such system here:
http://jcmvbkbc.org/~dumb/tmp/202403211236/README
it uses a prebuilt rootfs image and a 6.8 kernel branch with two patches on top of it: one adds a dts and a defconfig and the other is this fix. The rootfs boots successfully with this fix, but panics if this fix is removed.
The easiest way to actually see the AT_EXECFN is, I guess, to do something like that:
---8<---

fs/binfmt_elf_fdpic.c +0 -0

index fefc642541cb..22d34272a570 100644
@@ -659,6 +659,7 @@ static int create_elf_fdpic_tables(struct

linux_binprm *bprm,
NEW_AUX_ENT(AT_EGID, (elf_addr_t)

from_kgid_munged(cred->user_ns, cred->egid));
       NEW_AUX_ENT(AT_SECURE,  bprm->secureexec);
       NEW_AUX_ENT(AT_EXECFN,  bprm->exec);

+ pr_info("%s: AT_EXECFN = %#lx\n", __func__, bprm->exec);

#ifdef ARCH_DLINFO
       nr = 0;

---8<---

Quoted 1 line

How did you encounter the problem?

I'm doing xtensa FDPIC port of musl libc and this issue popped up when I began testing it on qemu-system-xtensa with the real linux kernel. Related post to the musl ML:
https://www.openwall.com/lists/musl/2024/03/20/2

--
Thanks.
-- Max

Kees Cook

2024-03-22 03:53 UTC #5 lore raw

Quoted 18 lines · Max Filippov

Quoted 11 lines · Kees Cook
Quoted 9 lines · Max Filippov
In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.
Quoted 5 lines
Quoted 2 lines
Adjust linux_binprm::exec before the successful return from the transfer_args_to_stack().
What's the best way to test this? (Is there a qemu setup I can use to see the before/after of AT_EXECFN?)

Quoted 6 lines

I put a readme with the steps to build such system here:
http://jcmvbkbc.org/~dumb/tmp/202403211236/README
it uses a prebuilt rootfs image and a 6.8 kernel branch with two patches on top of it: one adds a dts and a defconfig and the other is this fix. The rootfs boots successfully with this fix, but panics if this fix is removed.

Ah, perfect! Thanks for this.

Quoted 17 lines

The easiest way to actually see the AT_EXECFN is, I guess, to do something like that:
---8<---

fs/binfmt_elf_fdpic.c +1 -0

index fefc642541cb..22d34272a570 100644
@@ -659,6 +659,7 @@ static int create_elf_fdpic_tables(struct
 linux_binprm *bprm,
       NEW_AUX_ENT(AT_EGID,    (elf_addr_t)
 from_kgid_munged(cred->user_ns, cred->egid));
       NEW_AUX_ENT(AT_SECURE,  bprm->secureexec);
       NEW_AUX_ENT(AT_EXECFN,  bprm->exec);
+       pr_info("%s: AT_EXECFN = %#lx\n", __func__, bprm->exec);

#ifdef ARCH_DLINFO
       nr = 0;

Quoted 1 line

---8<---

Does musl have something like the LD_SHOW_AUXV env variable. With glibc, I usually explore auxv like so:

$ LD_SHOW_AUXV=1 uname -a | grep EXECFN
AT_EXECFN:            /usr/bin/uname

Quoted 6 lines

Quoted 1 line
How did you encounter the problem?
I'm doing xtensa FDPIC port of musl libc and this issue popped up when I began testing it on qemu-system-xtensa with the real linux kernel. Related post to the musl ML:
https://www.openwall.com/lists/musl/2024/03/20/2

Thanks!

Signature · 1 line

Kees Cook

Max Filippov

author

2024-03-22 16:39 UTC #6 lore raw

Quoted 12 lines · Kees Cook

Quoted 10 lines · Max Filippov
Quoted 2 lines · Kees Cook
What's the best way to test this? (Is there a qemu setup I can use to see the before/after of AT_EXECFN?)
I put a readme with the steps to build such system here:
http://jcmvbkbc.org/~dumb/tmp/202403211236/README
it uses a prebuilt rootfs image and a 6.8 kernel branch with two patches on top of it: one adds a dts and a defconfig and the other is this fix. The rootfs boots successfully with this fix, but panics if this fix is removed.

Quoted 5 lines

Does musl have something like the LD_SHOW_AUXV env variable. With glibc, I usually explore auxv like so:
$ LD_SHOW_AUXV=1 uname -a | grep EXECFN
AT_EXECFN:            /usr/bin/uname

I couldn't find anything like that in either musl or uClibc-ng. So I updated the above rootfs and put the following program into it as /bin/test-auxv:

#include <stdio.h>
#include <sys/auxv.h>

int main()

{
       unsigned long p = getauxval(AT_EXECFN);
       fprintf(stderr, "AT_EXECFN: 0x%lx\n", p);
       if (p)
               fprintf(stderr, "%s\n", (const char *)p);
       return 0;
}

While looking at it I also noticed that /proc/<pid>/auxv is empty on NOMMU, looks like there will be yet another fix for that.

Signature · 2 lines

Thanks.
-- Max

Kees Cook

Applied

2024-03-22 03:24 UTC #7 lore raw

In reply to Max Filippov

Quoted 10 lines · Max Filippov

In NUMMU kernel the value of linux_binprm::p is the offset inside the temporary program arguments array maintained in separate pages in the linux_binprm::page. linux_binprm::exec being a copy of linux_binprm::p thus must be adjusted when that array is copied to the user stack. Without that adjustment the value passed by the NOMMU kernel to the ELF program in the AT_EXECFN entry of the aux array doesn't make any sense and it may break programs that try to access memory pointed to by that entry.
[...]

Applied to for-next/execve, thanks!

[1/1] exec: fix linux_binprm::exec in transfer_args_to_stack()

      https://git.kernel.org/kees/c/2aea94ac14d1

Take care,

Signature · 1 line

Kees Cook