Message-ID: <CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com>
Date:   Wed, 13 Mar 2019 15:58:10 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Ali Saidi <alisaidi@...zon.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        X86 ML <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Will Deacon <will.deacon@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Anthony Liguori <aliguori@...zon.com>
Subject: 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 <alisaidi@...zon.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...)

-- 
Kees Cook

Powered by blists - more mailing lists