Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753325AbdFURdF (ORCPT ); Wed, 21 Jun 2017 13:33:05 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:36547 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751130AbdFURdD (ORCPT ); Wed, 21 Jun 2017 13:33:03 -0400 MIME-Version: 1.0 In-Reply-To: <1498066333.27465.8.camel@gmail.com> References: <20170621055835.GA27467@beast> <1498046876.13083.17.camel@redhat.com> <1498066040.27465.7.camel@gmail.com> <1498066333.27465.8.camel@gmail.com> From: Kees Cook Date: Wed, 21 Jun 2017 10:33:01 -0700 X-Google-Sender-Auth: 21gor1M0cH5wNb4d7aDOVCU8Mu0 Message-ID: Subject: Re: [kernel-hardening] [PATCH] [RFC] binfmt_elf: Use ELF_ET_DYN_BASE only for PIE To: Daniel Micay Cc: Rik van Riel , LKML , Qualys Security Advisory , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , Alexander Viro , Dmitry Safonov , Masahiro Yamada , Grzegorz Andrejczuk , "linux-fsdevel@vger.kernel.org" , "kernel-hardening@lists.openwall.com" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2906 Lines: 84 On Wed, Jun 21, 2017 at 10:32 AM, Daniel Micay wrote: > On Wed, 2017-06-21 at 10:28 -0700, Kees Cook wrote: >> On Wed, Jun 21, 2017 at 10:27 AM, Daniel Micay >> 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 >> > > 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 > > I think it was dropped when the ET_DYN changes were dropped. Ah! Rik, can you resend just the stack calculation fixes? I can Ack those. -Kees -- Kees Cook Pixel Security