linux-kernel - Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <74755222-0B16-4A81-85F6-D2803E4C0334@amacapital.net>
Date:   Thu, 18 Apr 2019 07:14:54 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Kees Cook <keescook@...omium.org>,
        Hector Marco-Gisbert <hecmargi@....es>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
        Brian Gerst <brgerst@...il.com>,
        Andy Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...e.de>,
        Huaitong Han <huaitong.han@...el.com>,
        Ismael Ripoll Ripoll <iripoll@....es>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>,
        Jason Gunthorpe <jgg@...lanox.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable


> On Apr 18, 2019, at 1:17 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> 
>> On Thu, 18 Apr 2019, Kees Cook wrote:
>> On Wed, May 11, 2016 at 5:45 AM Hector Marco-Gisbert <hecmargi@....es> wrote:
>> *thread necromancy*
>> 
>> I'd still like to see this get landed. READ_IMPLIES_EXEC is way too
>> powerful (it impacts, for example, mmap() regions of device driver
>> memory, forcing drivers to not be able to disallow VM_EXEC[1]).
>> 
>> The only case it could break is on an AMD K8 (Athlon only, I assume?),
>> which seems unlikely to have a modern kernel run on it. If there is
>> still concern, then we could just test against the NX CPU feature:
>> 
>> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
>> index 69c0f892e310..367cd36259a4 100644
>> --- a/arch/x86/include/asm/elf.h
>> +++ b/arch/x86/include/asm/elf.h
>> @@ -280,10 +280,12 @@ extern u32 elf_hwcap2;
>> 
>> /*
>>  * An executable for which elf_read_implies_exec() returns TRUE will
>> - * have the READ_IMPLIES_EXEC personality flag set automatically.
>> + * have the READ_IMPLIES_EXEC personality flag set automatically when
>> + * a CPU did not support NX or is using a 32-bit memory layout.
>>  */
>> -#define elf_read_implies_exec(ex, executable_stack)    \
>> -       (executable_stack != EXSTACK_DISABLE_X)
>> +#define elf_read_implies_exec(ex, executable_stack)            \
>> +       (mmap_is_ia32() || !(__supported_pte_mask & _PAGE_NX) ? \
> 
> What's special about ia32? All what matters is whether PAGE_NX is supported
> or not. That has nothing to do with 32/64bit unless I'm missing something
> (as usual).
> 
> 

I have the opposite question: who cares if we have NX?  On a CPU without NX, read implies exec, full stop. Why should nasty personality stuff matter at all?  The personality stuff is about supporting old crufty binaries.

So: are there old 64-bit binaries that have their stacks marked RX that expect mmap to automatically return X memory?  If so, then the patch is a problem. If not, then maybe the patch is okay.

All that being said, the comment in the patch seems to be highly misleading.  If the patch is to be applied, the comment needs serious work.