Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
* Saidi, Ali wrote: > On 3/27/19, 2:52 PM, "linux-arm-kernel on behalf of Kees Cook" > keesc...@chromium.org> wrote: > > Adding some more people to CC... what do people think about this > moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything > that worked 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. > > Kees' proposal addresses the issue for me. Anyone have concerns on this > proposed solution? I'd suggest incorporating all feedback and sending a v2 series - it's much easier to get people's attention via code submitted. ;-) Thanks, Ingo
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On Fri, Apr 19, 2019 at 3:51 AM Ingo Molnar wrote: > I'd suggest incorporating all feedback and sending a v2 series - it's > much easier to get people's attention via code submitted. ;-) I sent my patch out, akpm added it to -mm and then xtensa broke, and it got removed. S... I'm looking at it again now. I think I might combine the ideas and in the interpreter case bump the entire mmap base by the brk randomization size. That should make things less strange and solve the corner case without reducing available address space in the general case. -- Kees Cook
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On 3/27/19, 2:52 PM, "linux-arm-kernel on behalf of Kees Cook" wrote: Adding some more people to CC... what do people think about this moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything that worked 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. Kees' proposal addresses the issue for me. Anyone have concerns on this proposed solution? Ali
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On Wed, Mar 13, 2019 at 3:58 PM Kees Cook wrote:
>
> On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi wrote:
> >
> > Increase mmap_base by the worst-case brk randomization so that
> > the stack and heap remain apart.
> >
> > In Linux 4.13 a change was committed that special cased the kernel ELF
> > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> > directly and this issue is limited to cases where it is, (e.g to set a
> > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> > those rare cases, the loader doesn't take into account the amount of brk
> > randomization that will be applied by arch_randomize_brk(). This can
> > lead to the stack and heap being arbitrarily close to each other.
>
> 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
> 4000-c000 r-xp fd:02 34603015
> /bin/cat
> 5575b000-5575c000 r--p 7000 fd:02 34603015
> /bin/cat
> 5575c000-5575d000 rw-p 8000 fd:02 34603015
> /bin/cat
> 5575d000-5577e000 rw-p 00:00 0
> [heap]
> ...
> 77ff7000-77ffa000 r--p 00:00 0
> [vvar]
> 77ffa000-77ffc000 r-xp 00:00 0
> [vdso]
> 77ffc000-77ffd000 r--p 00027000 fd:02 49287483
> /lib/x86_64-linux-gnu/ld-2.27.so
> 77ffd000-77ffe000 rw-p 00028000 fd:02 49287483
> /lib/x86_64-linux-gnu/ld-2.27.so
> 77ffe000-77fff000 rw-p 00:00 0
> 7ffde000-7000 rw-p 00:00 0
> [stack]
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> ...
> 77bcc000-77bd4000 r-xp fd:02 34603015
> /bin/cat
> 77bd4000-77dd3000 ---p 8000 fd:02 34603015
> /bin/cat
> 77dd3000-77dd4000 r--p 7000 fd:02 34603015
> /bin/cat
> 77dd4000-77dd5000 rw-p 8000 fd:02 34603015
> /bin/cat
> 77dd5000-77dfc000 r-xp fd:02 49287483
> /lib/x86_64-linux-gnu/ld-2.27.so
> 77fb2000-77fd6000 rw-p 00:00 0
> 77ff7000-77ffa000 r--p 00:00 0
> [vvar]
> 77ffa000-77ffc000 r-xp 00:00 0
> [vdso]
> 77ffc000-77ffd000 r--p 00027000 fd:02 49287483
> /lib/x86_64-linux-gnu/ld-2.27.so
> 77ffd000-77ffe000 rw-p 00028000 fd:02 49287483
> /lib/x86_64-linux-gnu/ld-2.27.so
> 77ffe000-7802 rw-p 00:00 0
> [heap]
> 7ffde000-7000 rw-p 00:00 0
> [stack]
>
> So I think changing this globally isn't the right solution (normally
> brk is between text and mmap). Adjusting the mmap base padding means
> we lose even more memory space. Perhaps it would be better if brk
> allocation would be placed before the mmap region (i.e. use
> ELF_ET_DYN_BASE). This seems to work for me:
>
> 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);
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> 56de3000-56e04000 rw-p 00:00 0
> [heap]
> 7f8467da9000-7f8467f9 r-xp fd:01 399295
> /lib/x86_64-linux-gnu/libc-2.27.so
> ...
> 7f846819a000-7f84681a2000 r-xp fd:01 263229
> /bin/cat
> ...
> 7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
> /lib/x86_64-linux-gnu/ld-2.27.so
> 7f84685cc000-7f84685cd000 rw-p 00:00 0
> 7ffce68f8000-7ffce6919000 rw-p 00:00 0
> [stack]
> 7ffce69f-7ffce69f3000 r--p 00:00 0
> [vvar]
> 7ffce69f3000-7ffce69f4000 r-xp 00:00 0
> [vdso]
>
> Does anyone see problems with this? (Note that ET_EXEC base is
> 0x40, so no collision there...)
Adding some more people to CC... what do people think
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On Tue, 26 Mar 2019, Saidi, Ali wrote: > On 3/21/19, 9:11 AM, "linux-arm-kernel on behalf of Thomas Gleixner" > t...@linutronix.de> wrote: > > On Tue, 12 Mar 2019, Ali Saidi wrote: > > > Increase mmap_base by the worst-case brk randomization so that > > the stack and heap remain apart. > > > > In Linux 4.13 a change was committed that special cased the kernel ELF > > loader when the loader is invoked directly (eab09532d400; binfmt_elf: > use > > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked > > directly and this issue is limited to cases where it is, (e.g to set a > > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In > > those rare cases, the loader doesn't take into account the amount of brk > > randomization that will be applied by arch_randomize_brk(). This can > > lead to the stack and heap being arbitrarily close to each other. > > That explains not why you need this change. What's the consequence of them > being close to each other? > > The process doesn't get it's requested stack size and stack allocations > could end up scribbling on the heap. And exactly that information wants to be in the changelog. Thanks, tglx
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On 3/21/19, 9:11 AM, "linux-arm-kernel on behalf of Thomas Gleixner" wrote: On Tue, 12 Mar 2019, Ali Saidi wrote: > Increase mmap_base by the worst-case brk randomization so that > the stack and heap remain apart. > > In Linux 4.13 a change was committed that special cased the kernel ELF > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked > directly and this issue is limited to cases where it is, (e.g to set a > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In > those rare cases, the loader doesn't take into account the amount of brk > randomization that will be applied by arch_randomize_brk(). This can > lead to the stack and heap being arbitrarily close to each other. That explains not why you need this change. What's the consequence of them being close to each other? The process doesn't get it's requested stack size and stack allocations could end up scribbling on the heap. Thanks, Ali
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On Tue, 12 Mar 2019, Ali Saidi wrote: > Increase mmap_base by the worst-case brk randomization so that > the stack and heap remain apart. > > In Linux 4.13 a change was committed that special cased the kernel ELF > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked > directly and this issue is limited to cases where it is, (e.g to set a > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In > those rare cases, the loader doesn't take into account the amount of brk > randomization that will be applied by arch_randomize_brk(). This can > lead to the stack and heap being arbitrarily close to each other. That explains not why you need this change. What's the consequence of them being close to each other? Thanks, tglx
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
> On Mar 13, 2019, at 11:26 AM, Dave Hansen wrote: > >> On 3/12/19 10:32 AM, Ali Saidi wrote: >> +/* Provide space for brk randomization */ >> +pad += SZ_32M; > > Just curious: Why is the padding in your other patch conditional on the > 32-bit vs. 64-bit apps, but here it's always 32M? Arm changes the amount of brk based on the process being 32 vs 64 bit. X86 doesn’t appear to do this. > > Also, did you hit this problem in practice somehow? Just debugging a crash when testing a version of a library I compiled. Ali
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi wrote:
>
> Increase mmap_base by the worst-case brk randomization so that
> the stack and heap remain apart.
>
> In Linux 4.13 a change was committed that special cased the kernel ELF
> loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> directly and this issue is limited to cases where it is, (e.g to set a
> non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> those rare cases, the loader doesn't take into account the amount of brk
> randomization that will be applied by arch_randomize_brk(). This can
> lead to the stack and heap being arbitrarily close to each other.
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
4000-c000 r-xp fd:02 34603015
/bin/cat
5575b000-5575c000 r--p 7000 fd:02 34603015
/bin/cat
5575c000-5575d000 rw-p 8000 fd:02 34603015
/bin/cat
5575d000-5577e000 rw-p 00:00 0 [heap]
...
77ff7000-77ffa000 r--p 00:00 0 [vvar]
77ffa000-77ffc000 r-xp 00:00 0 [vdso]
77ffc000-77ffd000 r--p 00027000 fd:02 49287483
/lib/x86_64-linux-gnu/ld-2.27.so
77ffd000-77ffe000 rw-p 00028000 fd:02 49287483
/lib/x86_64-linux-gnu/ld-2.27.so
77ffe000-77fff000 rw-p 00:00 0
7ffde000-7000 rw-p 00:00 0 [stack]
$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
...
77bcc000-77bd4000 r-xp fd:02 34603015
/bin/cat
77bd4000-77dd3000 ---p 8000 fd:02 34603015
/bin/cat
77dd3000-77dd4000 r--p 7000 fd:02 34603015
/bin/cat
77dd4000-77dd5000 rw-p 8000 fd:02 34603015
/bin/cat
77dd5000-77dfc000 r-xp fd:02 49287483
/lib/x86_64-linux-gnu/ld-2.27.so
77fb2000-77fd6000 rw-p 00:00 0
77ff7000-77ffa000 r--p 00:00 0 [vvar]
77ffa000-77ffc000 r-xp 00:00 0 [vdso]
77ffc000-77ffd000 r--p 00027000 fd:02 49287483
/lib/x86_64-linux-gnu/ld-2.27.so
77ffd000-77ffe000 rw-p 00028000 fd:02 49287483
/lib/x86_64-linux-gnu/ld-2.27.so
77ffe000-7802 rw-p 00:00 0 [heap]
7ffde000-7000 rw-p 00:00 0 [stack]
So I think changing this globally isn't the right solution (normally
brk is between text and mmap). Adjusting the mmap base padding means
we lose even more memory space. Perhaps it would be better if brk
allocation would be placed before the mmap region (i.e. use
ELF_ET_DYN_BASE). This seems to work for me:
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);
$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
56de3000-56e04000 rw-p 00:00 0 [heap]
7f8467da9000-7f8467f9 r-xp fd:01 399295
/lib/x86_64-linux-gnu/libc-2.27.so
...
7f846819a000-7f84681a2000 r-xp fd:01 263229
/bin/cat
...
7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
/lib/x86_64-linux-gnu/ld-2.27.so
7f84685cc000-7f84685cd000 rw-p 00:00 0
7ffce68f8000-7ffce6919000 rw-p 00:00 0 [stack]
7ffce69f-7ffce69f3000 r--p 00:00 0 [vvar]
7ffce69f3000-7ffce69f4000 r-xp 00:00 0 [vdso]
Does anyone see problems with this? (Note that ET_EXEC base is
0x40, so no collision there...)
--
Kees Cook
Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
On 3/12/19 10:32 AM, Ali Saidi wrote: > + /* Provide space for brk randomization */ > + pad += SZ_32M; Just curious: Why is the padding in your other patch conditional on the 32-bit vs. 64-bit apps, but here it's always 32M? Also, did you hit this problem in practice somehow?
[PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
Increase mmap_base by the worst-case brk randomization so that the stack and heap remain apart. In Linux 4.13 a change was committed that special cased the kernel ELF loader when the loader is invoked directly (eab09532d400; binfmt_elf: use ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked directly and this issue is limited to cases where it is, (e.g to set a non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In those rare cases, the loader doesn't take into account the amount of brk randomization that will be applied by arch_randomize_brk(). This can lead to the stack and heap being arbitrarily close to each other. Signed-off-by: Ali Saidi --- arch/x86/mm/mmap.c | 4 1 file changed, 4 insertions(+) diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c index db3165714521..98a2875c37e3 100644 --- a/arch/x86/mm/mmap.c +++ b/arch/x86/mm/mmap.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include "physaddr.h" @@ -97,6 +98,9 @@ static unsigned long mmap_base(unsigned long rnd, unsigned long task_size, unsigned long pad = stack_maxrandom_size(task_size) + stack_guard_gap; unsigned long gap_min, gap_max; + /* Provide space for brk randomization */ + pad += SZ_32M; + /* Values close to RLIM_INFINITY can overflow. */ if (gap + pad > gap) gap += pad; -- 2.15.3.AMZN
