| 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
| ||
|
Message-ID: <c03a9255-0d5e-2715-51bd-f9fc143539cc@redhat.com>
Date: Tue, 9 Aug 2016 21:07:17 +0200
From: Denys Vlasenko <dvlasenk@...hat.com>
To: Kees Cook <keescook@...omium.org>
Cc: "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Oleg Nesterov <oleg@...hat.com>,
Michael Ellerman <mpe@...erman.id.au>,
Florian Weimer <fweimer@...hat.com>,
Linux-MM <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] powerpc: Do not make the entire heap executable
On 08/08/2016 09:07 PM, Kees Cook wrote:
> On Mon, Aug 8, 2016 at 7:55 AM, Denys Vlasenko <dvlasenk@...hat.com> wrote:
>> On 32-bit powerps the ELF PLT sections of binaries (built with --bss-plt,
>> or with a toolchain which defaults to it) look like this:
>>
>> [17] .sbss NOBITS 0002aff8 01aff8 000014 00 WA 0 0 4
>> [18] .plt NOBITS 0002b00c 01aff8 000084 00 WAX 0 0 4
>> [19] .bss NOBITS 0002b090 01aff8 0000a4 00 WA 0 0 4
>>
>> Which results in an ELF load header:
>>
>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>> LOAD 0x019c70 0x00029c70 0x00029c70 0x01388 0x014c4 RWE 0x10000
>>
>> This is all correct, the load region containing the PLT is marked as
>> executable. Note that the PLT starts at 0002b00c but the file mapping ends at
>> 0002aff8, so the PLT falls in the 0 fill section described by the load header,
>> and after a page boundary.
>>
>> Unfortunately the generic ELF loader ignores the X bit in the load headers
>> when it creates the 0 filled non-file backed mappings. It assumes all of these
>> mappings are RW BSS sections, which is not the case for PPC.
>>
>> gcc/ld has an option (--secure-plt) to not do this, this is said to incur
>> a small performance penalty.
>>
>> Currently, to support 32-bit binaries with PLT in BSS kernel maps *entire
>> brk area* with executable rights for all binaries, even --secure-plt ones.
>>
>> Stop doing that.
>>
>> Teach the ELF loader to check the X bit in the relevant load header
>> and create 0 filled anonymous mappings that are executable
>> if the load header requests that.
>>
>> The patch was originally posted in 2012 by Jason Gunthorpe
>> and apparently ignored:
>>
>> https://lkml.org/lkml/2012/9/30/138
>>
>> Lightly run-tested.
>>
>> Signed-off-by: Jason Gunthorpe <jgunthorpe@...idianresearch.com>
>> Signed-off-by: Denys Vlasenko <dvlasenk@...hat.com>
>> CC: Benjamin Herrenschmidt <benh@...nel.crashing.org>
>> CC: Paul Mackerras <paulus@...ba.org>
>> CC: Kees Cook <keescook@...omium.org>
>> CC: Oleg Nesterov <oleg@...hat.com>,
>> CC: Michael Ellerman <mpe@...erman.id.au>
>> CC: Florian Weimer <fweimer@...hat.com>
>> CC: linux-mm@...ck.org,
>> CC: linuxppc-dev@...ts.ozlabs.org
>> CC: linux-kernel@...r.kernel.org
>> ---
>> Changes since v1:
>> * wrapped lines to not exceed 79 chars
>> * improved comment
>> * expanded CC list
>>
>> arch/powerpc/include/asm/page.h | 10 +------
>> arch/powerpc/include/asm/page_32.h | 2 --
>> arch/powerpc/include/asm/page_64.h | 4 ---
>> fs/binfmt_elf.c | 56 ++++++++++++++++++++++++++++++--------
>> 4 files changed, 45 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index 56398e7..42d7ea1 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -225,15 +225,7 @@ extern long long virt_phys_offset;
>> #endif
>> #endif
>>
>> -/*
>> - * Unfortunately the PLT is in the BSS in the PPC32 ELF ABI,
>> - * and needs to be executable. This means the whole heap ends
>> - * up being executable.
>> - */
>> -#define VM_DATA_DEFAULT_FLAGS32 (VM_READ | VM_WRITE | VM_EXEC | \
>> - VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
>> -
>> -#define VM_DATA_DEFAULT_FLAGS64 (VM_READ | VM_WRITE | \
>> +#define VM_DATA_DEFAULT_FLAGS (VM_READ | VM_WRITE | \
>> VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
>>
>> #ifdef __powerpc64__
>> diff --git a/arch/powerpc/include/asm/page_32.h b/arch/powerpc/include/asm/page_32.h
>> index 6a8e179..6113fa8 100644
>> --- a/arch/powerpc/include/asm/page_32.h
>> +++ b/arch/powerpc/include/asm/page_32.h
>> @@ -9,8 +9,6 @@
>> #endif
>> #endif
>>
>> -#define VM_DATA_DEFAULT_FLAGS VM_DATA_DEFAULT_FLAGS32
>> -
>> #ifdef CONFIG_NOT_COHERENT_CACHE
>> #define ARCH_DMA_MINALIGN L1_CACHE_BYTES
>> #endif
>> diff --git a/arch/powerpc/include/asm/page_64.h b/arch/powerpc/include/asm/page_64.h
>> index dd5f071..52d8e9c 100644
>> --- a/arch/powerpc/include/asm/page_64.h
>> +++ b/arch/powerpc/include/asm/page_64.h
>> @@ -159,10 +159,6 @@ do { \
>>
>> #endif /* !CONFIG_HUGETLB_PAGE */
>>
>> -#define VM_DATA_DEFAULT_FLAGS \
>> - (is_32bit_task() ? \
>> - VM_DATA_DEFAULT_FLAGS32 : VM_DATA_DEFAULT_FLAGS64)
>> -
>> /*
>> * This is the default if a program doesn't have a PT_GNU_STACK
>> * program header entry. The PPC64 ELF ABI has a non executable stack
>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> index a7a28110..50006d0 100644
>> --- a/fs/binfmt_elf.c
>> +++ b/fs/binfmt_elf.c
>> @@ -91,14 +91,25 @@ static struct linux_binfmt elf_format = {
>>
>> #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
>>
>> -static int set_brk(unsigned long start, unsigned long end)
>> +static int set_brk(unsigned long start, unsigned long end, int prot)
>> {
>> start = ELF_PAGEALIGN(start);
>> end = ELF_PAGEALIGN(end);
>> if (end > start) {
>> - int error = vm_brk(start, end - start);
>> - if (error)
>> - return error;
>> + /* Map the non-file portion of the last load header. If the
>> + header is requesting these pages to be executeable then
>> + we have to honour that, otherwise assume they are bss. */
>> + if (prot & PROT_EXEC) {
>> + unsigned long addr;
>> + addr = vm_mmap(0, start, end - start, prot,
>> + MAP_PRIVATE | MAP_FIXED, 0);
>> + if (BAD_ADDR(addr))
>> + return addr;
>> + } else {
>> + int error = vm_brk(start, end - start);
>> + if (error)
>> + return error;
>> + }
>
> Rather than repeating this logic twice, I think this should be a
> helper that both sections can call.
I propose to teach vm_brk() to be able to optionally set some VM_foo bits,
such as VM_EXEC. Then there will be no need for conditional vm_mmap/vm_brk here.
Patch v3 is on the way.
Powered by blists - more mailing lists