Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190312173248.13490-1-alisaidi@amazon.com> <20190312173248.13490-3-alisaidi@amazon.com>
 <CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com>
In-Reply-To: <CAGXu5jJ5sj3emOT2QPxQkNQk0qbU6zEfu9=Omfhx_p0nCKPSjA@mail.gmail.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Wed, 27 Mar 2019 12:51:33 -0700
Message-ID: <CAGXu5jKcBmj0kwAqjqROXuOi=90=Upy8-J8WviGMjD6FV_Guxg@mail.gmail.com>
Subject: Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
To:     Ali Saidi <alisaidi@amazon.com>, Michal Hocko <mhocko@suse.com>,
        Matthew Wilcox <willy@infradead.org>,
        Jann Horn <jannh@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        X86 ML <x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Will Deacon <will.deacon@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Anthony Liguori <aliguori@amazon.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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: u=
se
> > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn=E2=80=99t invo=
ked
> > 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). I=
n
> > those rare cases, the loader doesn't take into account the amount of br=
k
> > 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 *bp=
rm)
>         current->mm->end_data =3D end_data;
>         current->mm->start_stack =3D 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 ma=
y
> +        * collide early with the stack growing down), and into the unuse=
d
> +        * ELF_ET_DYN_BASE region.
> +        */
> +       if (!elf_interpreter)
> +               current->mm->brk =3D current->mm->start_brk =3D ELF_ET_DY=
N_BASE;
> +
>         if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) =
{
>                 current->mm->brk =3D current->mm->start_brk =3D
>                         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.

--=20
Kees Cook