lkml.org 
[lkml]   [2019]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
    On Wed, Mar 13, 2019 at 3:58 PM Kees Cook <keescook@chromium.org> wrote:
    >
    > On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi <alisaidi@amazon.com> 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
    > 555555554000-55555555c000 r-xp 00000000 fd:02 34603015
    > /bin/cat
    > 55555575b000-55555575c000 r--p 00007000 fd:02 34603015
    > /bin/cat
    > 55555575c000-55555575d000 rw-p 00008000 fd:02 34603015
    > /bin/cat
    > 55555575d000-55555577e000 rw-p 00000000 00:00 0 [heap]
    > ...
    > 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0 [vvar]
    > 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0 [vdso]
    > 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
    > /lib/x86_64-linux-gnu/ld-2.27.so
    > 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
    > /lib/x86_64-linux-gnu/ld-2.27.so
    > 7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
    > 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0 [stack]
    >
    > $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
    > ...
    > 7ffff7bcc000-7ffff7bd4000 r-xp 00000000 fd:02 34603015
    > /bin/cat
    > 7ffff7bd4000-7ffff7dd3000 ---p 00008000 fd:02 34603015
    > /bin/cat
    > 7ffff7dd3000-7ffff7dd4000 r--p 00007000 fd:02 34603015
    > /bin/cat
    > 7ffff7dd4000-7ffff7dd5000 rw-p 00008000 fd:02 34603015
    > /bin/cat
    > 7ffff7dd5000-7ffff7dfc000 r-xp 00000000 fd:02 49287483
    > /lib/x86_64-linux-gnu/ld-2.27.so
    > 7ffff7fb2000-7ffff7fd6000 rw-p 00000000 00:00 0
    > 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0 [vvar]
    > 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0 [vdso]
    > 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
    > /lib/x86_64-linux-gnu/ld-2.27.so
    > 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
    > /lib/x86_64-linux-gnu/ld-2.27.so
    > 7ffff7ffe000-7ffff8020000 rw-p 00000000 00:00 0 [heap]
    > 7ffffffde000-7ffffffff000 rw-p 00000000 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
    > 555556de3000-555556e04000 rw-p 00000000 00:00 0 [heap]
    > 7f8467da9000-7f8467f90000 r-xp 00000000 fd:01 399295
    > /lib/x86_64-linux-gnu/libc-2.27.so
    > ...
    > 7f846819a000-7f84681a2000 r-xp 00000000 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 00000000 00:00 0
    > 7ffce68f8000-7ffce6919000 rw-p 00000000 00:00 0 [stack]
    > 7ffce69f0000-7ffce69f3000 r--p 00000000 00:00 0 [vvar]
    > 7ffce69f3000-7ffce69f4000 r-xp 00000000 00:00 0 [vdso]
    >
    > Does anyone see problems with this? (Note that ET_EXEC base is
    > 0x400000, so no collision there...)

    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 Cook

    \
     
     \ /
      Last update: 2019-03-27 20:52    [W:3.157 / U:0.380 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site