[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 21 Jun 2017 10:28:11 -0700
From: Kees Cook <keescook@...omium.org>
To: Daniel Micay <danielmicay@...il.com>
Cc: Rik van Riel <riel@...hat.com>, 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, Jun 21, 2017 at 10:27 AM, Daniel Micay <danielmicay@...il.com> wrote:
> 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?
Yes. Was that picked up by akpm already?
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
