[PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[Kees Cook]

Commit eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE"),
made changes in the rare case when the ELF loader was directly invoked
(e.g to set a non-inheritable LD_LIBRARY_PATH, testing new versions of
the loader), by moving into the mmap region to avoid both ET_EXEC and PIE
binaries. This had the effect of also moving the brk region into mmap,
which could lead to the stack and brk being arbitrarily close to each
other. An unlucky process wouldn't get its requested stack size and stack
allocations could end up scribbling on the heap.

This is illustrated here. In the case of using the loader directly, brk
(so helpfully identified as "[heap]") is allocated with the _loader_
not the binary. For example, with ASLR entirely disabled, you can see
this more clearly:

$ /bin/cat /proc/self/maps
555555554000-55555555c000 r-xp 00000000 ... /bin/cat
55555575b000-55555575c000 r--p 00007000 ... /bin/cat
55555575c000-55555575d000 rw-p 00008000 ... /bin/cat
55555575d000-55555577e000 rw-p 00000000 ... [heap]
...
7ffff7ff7000-7ffff7ffa000 r--p 00000000 ... [vvar]
7ffff7ffa000-7ffff7ffc000 r-xp 00000000 ... [vdso]
7ffff7ffc000-7ffff7ffd000 r--p 00027000 ... /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffd000-7ffff7ffe000 rw-p 00028000 ... /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffe000-7ffff7fff000 rw-p 00000000 ...
7ffffffde000-7ffffffff000 rw-p 00000000 ... [stack]

$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
...
7ffff7bcc000-7ffff7bd4000 r-xp 00000000 ... /bin/cat
7ffff7bd4000-7ffff7dd3000 ---p 00008000 ... /bin/cat
7ffff7dd3000-7ffff7dd4000 r--p 00007000 ... /bin/cat
7ffff7dd4000-7ffff7dd5000 rw-p 00008000 ... /bin/cat
7ffff7dd5000-7ffff7dfc000 r-xp 00000000 ... /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7fb2000-7ffff7fd6000 rw-p 00000000 ...
7ffff7ff7000-7ffff7ffa000 r--p 00000000 ... [vvar]
7ffff7ffa000-7ffff7ffc000 r-xp 00000000 ... [vdso]
7ffff7ffc000-7ffff7ffd000 r--p 00027000 ... /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffd000-7ffff7ffe000 rw-p 00028000 ... /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffe000-7ffff8020000 rw-p 00000000 ... [heap]
7ffffffde000-7ffffffff000 rw-p 00000000 ... [stack]

The solution is to move brk out of mmap and into ELF_ET_DYN_BASE since
nothing is there in this direct loader case (and ET_EXEC still far
away at 0x400000). Anything that ran before should still work (i.e. the
ultimately-launched binary already had the brk very far from its text,
so this should be no different from a COMPAT_BRK standpoint). The only
risk I see here is that if someone started to suddenly depend on the
entire memory space above the mmap region being available when launching
binaries via a direct loader execs which seems highly unlikely, I'd hope:
this would mean a binary would _not_ work when execed normally.

Reported-by: Ali Saidi <alisaidi@amazon.com>
Link: https://lkml.kernel.org/r/CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com
Fixes: eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/binfmt_elf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 7d09d125f148..cdaa33f4a3ef 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1131,6 +1131,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	current->mm->end_data = end_data;
 	current->mm->start_stack = bprm->p;
 
+	/*
+	 * When executing a loader directly (ET_DYN without Interp), move
+	 * the brk area out of the mmap region (since it grows up, and may
+	 * collide early with the stack growing down), and into the unused
+	 * ELF_ET_DYN_BASE region.
+	 */
+	if (!elf_interpreter)
+		current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
+
 	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
 		current->mm->brk = current->mm->start_brk =
 			arch_randomize_brk(current->mm);
-- 
2.17.1


-- 
Kees Cook

Re: [PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[Andrew Morton]

On Mon, 15 Apr 2019 21:23:20 -0700 Kees Cook <keescook@chromium.org> wrote:

> Commit eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE"),
> made changes in the rare case when the ELF loader was directly invoked
> (e.g to set a non-inheritable LD_LIBRARY_PATH, testing new versions of
> the loader), by moving into the mmap region to avoid both ET_EXEC and PIE
> binaries. This had the effect of also moving the brk region into mmap,
> which could lead to the stack and brk being arbitrarily close to each
> other. An unlucky process wouldn't get its requested stack size and stack
> allocations could end up scribbling on the heap.
> 
> This is illustrated here. In the case of using the loader directly, brk
> (so helpfully identified as "[heap]") is allocated with the _loader_
> not the binary. For example, with ASLR entirely disabled, you can see
> this more clearly:
> 
> $ /bin/cat /proc/self/maps
> 555555554000-55555555c000 r-xp 00000000 ... /bin/cat
> 55555575b000-55555575c000 r--p 00007000 ... /bin/cat
> 55555575c000-55555575d000 rw-p 00008000 ... /bin/cat
> 55555575d000-55555577e000 rw-p 00000000 ... [heap]
> ...
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 ... [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 ... [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff7fff000 rw-p 00000000 ...
> 7ffffffde000-7ffffffff000 rw-p 00000000 ... [stack]
> 
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> ...
> 7ffff7bcc000-7ffff7bd4000 r-xp 00000000 ... /bin/cat
> 7ffff7bd4000-7ffff7dd3000 ---p 00008000 ... /bin/cat
> 7ffff7dd3000-7ffff7dd4000 r--p 00007000 ... /bin/cat
> 7ffff7dd4000-7ffff7dd5000 rw-p 00008000 ... /bin/cat
> 7ffff7dd5000-7ffff7dfc000 r-xp 00000000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7fb2000-7ffff7fd6000 rw-p 00000000 ...
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 ... [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 ... [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff8020000 rw-p 00000000 ... [heap]
> 7ffffffde000-7ffffffff000 rw-p 00000000 ... [stack]
> 
> The solution is to move brk out of mmap and into ELF_ET_DYN_BASE since
> nothing is there in this direct loader case (and ET_EXEC still far
> away at 0x400000). Anything that ran before should still work (i.e. the
> ultimately-launched binary already had the brk very far from its text,
> so this should be no different from a COMPAT_BRK standpoint). The only
> risk I see here is that if someone started to suddenly depend on the
> entire memory space above the mmap region being available when launching
> binaries via a direct loader execs which seems highly unlikely, I'd hope:
> this would mean a binary would _not_ work when execed normally.
> 
> Reported-by: Ali Saidi <alisaidi@amazon.com>
> Link: https://lkml.kernel.org/r/CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com
> Fixes: eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")
> Signed-off-by: Kees Cook <keescook@chromium.org>

No cc:stable?

Re: [PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[Kees Cook]

On Tue, Apr 16, 2019 at 6:04 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 15 Apr 2019 21:23:20 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> > Commit eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE"),
> > made changes in the rare case when the ELF loader was directly invoked
> > (e.g to set a non-inheritable LD_LIBRARY_PATH, testing new versions of
> > the loader), by moving into the mmap region to avoid both ET_EXEC and PIE
> > binaries. This had the effect of also moving the brk region into mmap,
> > which could lead to the stack and brk being arbitrarily close to each
> > other. An unlucky process wouldn't get its requested stack size and stack
> > allocations could end up scribbling on the heap.
> >
> > This is illustrated here. In the case of using the loader directly, brk
> > (so helpfully identified as "[heap]") is allocated with the _loader_
> > not the binary. For example, with ASLR entirely disabled, you can see
> > this more clearly:
> >
> > $ /bin/cat /proc/self/maps
> > 555555554000-55555555c000 r-xp 00000000 ... /bin/cat
> > 55555575b000-55555575c000 r--p 00007000 ... /bin/cat
> > 55555575c000-55555575d000 rw-p 00008000 ... /bin/cat
> > 55555575d000-55555577e000 rw-p 00000000 ... [heap]
> > ...
> > 7ffff7ff7000-7ffff7ffa000 r--p 00000000 ... [vvar]
> > 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 ... [vdso]
> > 7ffff7ffc000-7ffff7ffd000 r--p 00027000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> > 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> > 7ffff7ffe000-7ffff7fff000 rw-p 00000000 ...
> > 7ffffffde000-7ffffffff000 rw-p 00000000 ... [stack]
> >
> > $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> > ...
> > 7ffff7bcc000-7ffff7bd4000 r-xp 00000000 ... /bin/cat
> > 7ffff7bd4000-7ffff7dd3000 ---p 00008000 ... /bin/cat
> > 7ffff7dd3000-7ffff7dd4000 r--p 00007000 ... /bin/cat
> > 7ffff7dd4000-7ffff7dd5000 rw-p 00008000 ... /bin/cat
> > 7ffff7dd5000-7ffff7dfc000 r-xp 00000000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> > 7ffff7fb2000-7ffff7fd6000 rw-p 00000000 ...
> > 7ffff7ff7000-7ffff7ffa000 r--p 00000000 ... [vvar]
> > 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 ... [vdso]
> > 7ffff7ffc000-7ffff7ffd000 r--p 00027000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> > 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 ... /lib/x86_64-linux-gnu/ld-2.27.so
> > 7ffff7ffe000-7ffff8020000 rw-p 00000000 ... [heap]
> > 7ffffffde000-7ffffffff000 rw-p 00000000 ... [stack]
> >
> > The solution is to move brk out of mmap and into ELF_ET_DYN_BASE since
> > nothing is there in this direct loader case (and ET_EXEC still far
> > away at 0x400000). Anything that ran before should still work (i.e. the
> > ultimately-launched binary already had the brk very far from its text,
> > so this should be no different from a COMPAT_BRK standpoint). The only
> > risk I see here is that if someone started to suddenly depend on the
> > entire memory space above the mmap region being available when launching
> > binaries via a direct loader execs which seems highly unlikely, I'd hope:
> > this would mean a binary would _not_ work when execed normally.
> >
> > Reported-by: Ali Saidi <alisaidi@amazon.com>
> > Link: https://lkml.kernel.org/r/CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com
> > Fixes: eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> No cc:stable?

Probably it should be, yes. I think I'm just shy about that when
poking ELF mappings. :)

-- 
Kees Cook

Re: [PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[Andrew Morton]

On Tue, 16 Apr 2019 18:14:00 -0500 Kees Cook <keescook@chromium.org> wrote:

> On Tue, Apr 16, 2019 at 6:04 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > >
> > > Reported-by: Ali Saidi <alisaidi@amazon.com>
> > > Link: https://lkml.kernel.org/r/CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com
> > > Fixes: eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > No cc:stable?
> 
> Probably it should be, yes. I think I'm just shy about that when
> poking ELF mappings. :)

Well, the -stable bots will backport anything that might look slightly
like a fix anyway.

I'll add cc:stable and shall hold it out until 5.2-rc1, so it should
get a bit of a spin before being backported.

Re: [PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[Guenter Roeck]

On Mon, Apr 15, 2019 at 09:23:20PM -0700, Kees Cook wrote:
> Commit eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE"),
> made changes in the rare case when the ELF loader was directly invoked
> (e.g to set a non-inheritable LD_LIBRARY_PATH, testing new versions of
> the loader), by moving into the mmap region to avoid both ET_EXEC and PIE
> binaries. This had the effect of also moving the brk region into mmap,
> which could lead to the stack and brk being arbitrarily close to each
> other. An unlucky process wouldn't get its requested stack size and stack
> allocations could end up scribbling on the heap.
> 

This patch results in crashes of my xtensa boot tests.

Run /sbin/init as init process
Kernel panic - not syncing: Attempted to kill init!  exitcode=0x0000000b

Bisect log is attached. The crash is seen in next-20190417 and next-20190418.

Guenter

---
# bad: [3f018f4a019a1110527910bac52161e57107957c] Add linux-next specific files for 20190418
# good: [15ade5d2e7775667cf191cf2f94327a4889f8b9d] Linux 5.1-rc4
git bisect start 'HEAD' 'v5.1-rc4'
# good: [d49e1f8649c84f154e7df59300264f59b736f329] Merge remote-tracking branch 'crypto/master'
git bisect good d49e1f8649c84f154e7df59300264f59b736f329
# good: [06a21957e5c0aae87fb94b97ef965818bd7c9dac] Merge remote-tracking branch 'spi/for-next'
git bisect good 06a21957e5c0aae87fb94b97ef965818bd7c9dac
# good: [c44f3caed068c67fe01056329e7e6cbf8f4920a8] Merge remote-tracking branch 'staging/staging-next'
git bisect good c44f3caed068c67fe01056329e7e6cbf8f4920a8
# good: [ca75e0d0aafd76ceb1f5a8f8544d22f841b7e296] Merge remote-tracking branch 'coresight/next'
git bisect good ca75e0d0aafd76ceb1f5a8f8544d22f841b7e296
# bad: [711a07489617c04954816b16d0d500b1e81398c1] cpumask-fix-double-string-traverse-in-cpumask_parse-fix
git bisect bad 711a07489617c04954816b16d0d500b1e81398c1
# good: [47d765617fc96a61b317dd916a48f28bec14b32c] mm/hmm: mirror hugetlbfs (snapshoting, faulting and DMA mapping)
git bisect good 47d765617fc96a61b317dd916a48f28bec14b32c
# good: [99b8edbb7d2564aa751481aae99a5715170b80ea] mm, memcg: make scan aggression always exclude protection
git bisect good 99b8edbb7d2564aa751481aae99a5715170b80ea
# good: [9174b7e52219aebd841086d67988295096bab871] lib/sort: avoid indirect calls to built-in swap
git bisect good 9174b7e52219aebd841086d67988295096bab871
# good: [b7a32277de780ace15114ce9f89c32f5b28276a0] fs/binfmt_elf.c: remove unneeded initialization of mm->start_stack
git bisect good b7a32277de780ace15114ce9f89c32f5b28276a0
# bad: [9100e2f3a64a66c366cd1f4b8f46d3e2d916ec54] fix "fs/binfmt_elf.c: move brk out of mmap when doing direct loader exec"
git bisect bad 9100e2f3a64a66c366cd1f4b8f46d3e2d916ec54
# good: [c1dcda60c739c3a4b3226317fca17dcd2827f604] fs/binfmt_elf.c: delete trailing "return;" in functions returning "void"
git bisect good c1dcda60c739c3a4b3226317fca17dcd2827f604
# good: [f39fe61cc3b48fe58ea1470ad7d6512cb45c76d3] fs//binfmt_elf.c: move variables initialization closer to their usage
git bisect good f39fe61cc3b48fe58ea1470ad7d6512cb45c76d3
# bad: [cb084e1ba0b0c3353e1fadf52f647ff85eb8989c] fs/binfmt_elf.c: move brk out of mmap when doing direct loader exec
git bisect bad cb084e1ba0b0c3353e1fadf52f647ff85eb8989c
# first bad commit: [cb084e1ba0b0c3353e1fadf52f647ff85eb8989c] fs/binfmt_elf.c: move brk out of mmap when doing direct loader exec

Re: [PATCH] binfmt_elf: Move brk out of mmap when doing direct loader exec

[Kees Cook]

On Thu, Apr 18, 2019 at 7:57 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Apr 15, 2019 at 09:23:20PM -0700, Kees Cook wrote:
> > Commit eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE"),
> > made changes in the rare case when the ELF loader was directly invoked
> > (e.g to set a non-inheritable LD_LIBRARY_PATH, testing new versions of
> > the loader), by moving into the mmap region to avoid both ET_EXEC and PIE
> > binaries. This had the effect of also moving the brk region into mmap,
> > which could lead to the stack and brk being arbitrarily close to each
> > other. An unlucky process wouldn't get its requested stack size and stack
> > allocations could end up scribbling on the heap.
> >
>
> This patch results in crashes of my xtensa boot tests.
>
> Run /sbin/init as init process
> Kernel panic - not syncing: Attempted to kill init!  exitcode=0x0000000b

Thanks for finding this! I *think* the issue is that I needed to be
testing for CONFIG_ARCH_HAS_ELF_RANDOMIZATION, which xtensa lacks.
I'll get this fixed up and resent through -mm.

-- 
Kees Cook