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

[PATCH v2] 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 the direct loader case (and ET_EXEC is 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 lower than 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 exec()ed normally.

(Note that this is only done under CONFIG_ARCH_HAS_ELF_RANDOMIZATION when
randomization is turned on.)

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>
---
v2: limit effect to only architectures that are expecting it! (Gunter)
---
 fs/binfmt_elf.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index fe5668a1bbaa..8cec7a97bfb7 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1131,16 +1131,18 @@ 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 (!interpreter)
-		current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
-
 	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
+		/*
+		 * For architectures with ELF randomization, when executing
+		 * a loader directly (i.e. no interpreter listed in ELF
+		 * headers), 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 (IS_ENABLED(CONFIG_ARCH_HAS_ELF_RANDOMIZE) && !interpreter)
+			current->mm->brk = current->mm->start_brk =
+				ELF_ET_DYN_BASE;
+
 		current->mm->brk = current->mm->start_brk =
 			arch_randomize_brk(current->mm);
 #ifdef compat_brk_randomized
-- 
2.17.1


-- 
Kees Cook

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

[Andrew Morton]

On Mon, 22 Apr 2019 15:57:27 -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.
>
> ...
>
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1131,16 +1131,18 @@ 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 (!interpreter)
> -		current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;

The above bit isn't there any more.  Here's what I queued:

--- a/fs/binfmt_elf.c~binfmt_elf-move-brk-out-of-mmap-when-doing-direct-loader-exec
+++ a/fs/binfmt_elf.c
@@ -1134,6 +1134,17 @@ out_free_interp:
 	current->mm->start_stack = bprm->p;
 
 	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
+		/*
+		 * For architectures with ELF randomization, when executing
+		 * a loader directly (i.e. no interpreter listed in ELF
+		 * headers), 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 (IS_ENABLED(CONFIG_ARCH_HAS_ELF_RANDOMIZE) && !interpreter)
+			current->mm->brk = current->mm->start_brk =
+				ELF_ET_DYN_BASE;
+
 		current->mm->brk = current->mm->start_brk =
 			arch_randomize_brk(current->mm);
 #ifdef compat_brk_randomized
_

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

[Guenter Roeck]

On 4/22/19 3:57 PM, 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 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 the direct loader case (and ET_EXEC is 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 lower than 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 exec()ed normally.
> 
> (Note that this is only done under CONFIG_ARCH_HAS_ELF_RANDOMIZATION when
> randomization is turned on.)
> 
> 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>
> ---
> v2: limit effect to only architectures that are expecting it! (Gunter)

This patch applies cleanly on top of next-20190418, and the crashes
observed with my xtensa boot tests no longer occur. I didn't test
any other architectures.

I don't know the base of Andrew's modification - the code snipped he says
isn't there anymore is still present in next-20190418.

Guenter