| 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: <CAGXu5j+HYkAweod-CC7QhTr3rB-18fMMzthdu4f_Cg3ykQZXUg@mail.gmail.com>
Date: Mon, 8 Aug 2016 12:07:05 -0700
From: Kees Cook <keescook@...omium.org>
To: Denys Vlasenko <dvlasenk@...hat.com>
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 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.
-Kees
> }
> current->mm->start_brk = current->mm->brk = end;
> return 0;
> @@ -524,6 +535,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> unsigned long load_addr = 0;
> int load_addr_set = 0;
> unsigned long last_bss = 0, elf_bss = 0;
> + int bss_prot = 0;
> unsigned long error = ~0UL;
> unsigned long total_size;
> int i;
> @@ -606,8 +618,10 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> * elf_bss and last_bss is the bss section.
> */
> k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
> - if (k > last_bss)
> + if (k > last_bss) {
> last_bss = k;
> + bss_prot = elf_prot;
> + }
> }
> }
>
> @@ -626,10 +640,27 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> /* What we have mapped so far */
> elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
>
> - /* Map the last of the bss segment */
> - error = vm_brk(elf_bss, last_bss - elf_bss);
> - if (error)
> - goto out;
> + if (last_bss > elf_bss) {
> + /*
> + * Map the non-file portion of the last load header.
> + * If the header is requesting these pages to be
> + * executable, honour that (ppc32 needs this).
> + * Otherwise assume they are bss.
> + */
> + if (bss_prot & PROT_EXEC) {
> + unsigned long addr;
> + addr = vm_mmap(0, elf_bss, last_bss - elf_bss,
> + bss_prot, MAP_PRIVATE | MAP_FIXED, 0);
> + if (BAD_ADDR(addr)) {
> + error = addr;
> + goto out;
> + }
> + } else {
> + error = vm_brk(elf_bss, last_bss - elf_bss);
> + if (error)
> + goto out;
> + }
> + }
> }
>
> error = load_addr;
> @@ -672,6 +700,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> unsigned long error;
> struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
> unsigned long elf_bss, elf_brk;
> + int bss_prot = 0;
> int retval, i;
> unsigned long elf_entry;
> unsigned long interp_load_addr = 0;
> @@ -879,7 +908,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
> before this one. Map anonymous pages, if needed,
> and clear the area. */
> retval = set_brk(elf_bss + load_bias,
> - elf_brk + load_bias);
> + elf_brk + load_bias,
> + bss_prot);
> if (retval)
> goto out_free_dentry;
> nbyte = ELF_PAGEOFFSET(elf_bss);
> @@ -973,8 +1003,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
> if (end_data < k)
> end_data = k;
> k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
> - if (k > elf_brk)
> + if (k > elf_brk) {
> + bss_prot = elf_prot;
> elf_brk = k;
> + }
> }
>
> loc->elf_ex.e_entry += load_bias;
> @@ -990,7 +1022,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> * mapping in the interpreter, to make sure it doesn't wind
> * up getting placed where the bss needs to go.
> */
> - retval = set_brk(elf_bss, elf_brk);
> + retval = set_brk(elf_bss, elf_brk, bss_prot);
> if (retval)
> goto out_free_dentry;
> if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bss))) {
> --
> 2.9.2
>
--
Kees Cook
Nexus Security
Powered by blists - more mailing lists