Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 21 Jun 2017 13:27:20 -0400
From: Daniel Micay <danielmicay@...il.com>
To: Kees Cook <keescook@...omium.org>, Rik van Riel <riel@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Qualys Security Advisory
 <qsa@...lys.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
 <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, "x86@...nel.org"
 <x86@...nel.org>, Alexander Viro <viro@...iv.linux.org.uk>, Dmitry Safonov
 <dsafonov@...tuozzo.com>, Masahiro Yamada <yamada.masahiro@...ionext.com>, 
 Grzegorz Andrejczuk <grzegorz.andrejczuk@...el.com>,
 "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, 
 "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH] [RFC] binfmt_elf: Use
 ELF_ET_DYN_BASE only for PIE

On Wed, 2017-06-21 at 10:23 -0700, Kees Cook wrote:
> On Wed, Jun 21, 2017 at 5:07 AM, Rik van Riel <riel@...hat.com> wrote:
> > On Tue, 2017-06-20 at 22:58 -0700, Kees Cook wrote:
> > > +/* This is the base location for PIE (ET_DYN with INTERP) loads.
> > > */
> > > +#define ELF_ET_DYN_BASE              0x400000UL
> > 
> > This value is good for 32 bit binaries, but for 64
> > bit binaries you probably want to put ELF_ET_DYN_BASE
> > at 4GB or higher.
> > 
> > The latter is necessary because Android uses the
> > lower 4GB of address space for its JVM runtime,
> > with 32 bit pointers inside that part of the otherwise
> > 64 bit address space.
> > 
> > In other words:
> > 
> > #define ELF_ET_DYN_BASE (mmap_is_ia32() ? 0x400000UL :
> > 0x100000000UL)
> 
> Ah, interesting. Okay, that should be fine. I'll adjust it.
> 
> > > +++ b/fs/binfmt_elf.c
> > > 
> > > +                      * Therefore, programs are loaded offset
> > > from
> > > +                      * ELF_ET_DYN_BASE and loaders are loaded
> > > into the
> > > +                      * independently randomized mmap region (0
> > > load_bias
> > > +                      * without MAP_FIXED).
> > > +                      */
> > > +                     if (elf_interpreter) {
> > > +                             load_bias = ELF_ET_DYN_BASE;
> > > +                             if (current->flags & PF_RANDOMIZE)
> > > +                                     load_bias +=
> > > arch_mmap_rnd();
> > > +                             elf_flags |= MAP_FIXED;
> > > +                     } else
> > > +                             load_bias = 0;
> > > +
> > > +                     load_bias -= vaddr;
> > 
> > I like this, and the big comment telling people how it
> > works :)
> 
> Thanks! It looks like your patch for commenting load_bias never got
> picked up, so I've added some more comments for that and some other
> things too. (Mostly for all the stuff I have to read a few times when
> I look at this code.)
> 
> -Kees
> 

The stack rlimit calculation fix for space potentially lost to ASLR is
probably still needed too, right?

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.