subject:"\[PATCH v3\] powerpc\: Do not make the entire heap executable"

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-10 Thread Denys Vlasenko




On 08/10/2016 06:36 AM, Michael Ellerman wrote:

Denys Vlasenko  writes:


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:

...


 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| 34 ++
 include/linux/mm.h |  1 +
 mm/mmap.c  | 20 +++-
 6 files changed, 43 insertions(+), 28 deletions(-)


What tree is this against?


Linus' tree from before August 2.
The "mm: refuse wrapped vm_brk requests" commit collided with my changes
I'll send patch v4 rebased to today's linus tree as soon as I finish testing it.

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-10 Thread Denys Vlasenko




On 08/10/2016 06:36 AM, Michael Ellerman wrote:

Denys Vlasenko  writes:


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:

...


 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| 34 ++
 include/linux/mm.h |  1 +
 mm/mmap.c  | 20 +++-
 6 files changed, 43 insertions(+), 28 deletions(-)


What tree is this against?


Linus' tree from before August 2.
The "mm: refuse wrapped vm_brk requests" commit collided with my changes
I'll send patch v4 rebased to today's linus tree as soon as I finish testing it.

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-10 Thread Denys Vlasenko


On 08/10/2016 12:43 AM, Kees Cook wrote:

-static int do_brk(unsigned long addr, unsigned long len)
+static int do_brk_flags(unsigned long addr, unsigned long len, unsigned long 
flags)
 {
struct mm_struct *mm = current->mm;
struct vm_area_struct *vma, *prev;
-   unsigned long flags;
struct rb_node **rb_link, *rb_parent;
pgoff_t pgoff = addr >> PAGE_SHIFT;
int error;
@@ -2666,7 +2665,7 @@ static int do_brk(unsigned long addr, unsigned long len)
if (!len)
return 0;

-   flags = VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;
+   flags |= VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;


For sanity's sake, should a mask be applied here? i.e. to be extra
careful about what flags can get passed in?


Maybe... I am leaving it to mm experts.


Otherwise, this looks okay to me:

Reviewed-by: Kees Cook 

-Kees

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-10 Thread Denys Vlasenko


On 08/10/2016 12:43 AM, Kees Cook wrote:

-static int do_brk(unsigned long addr, unsigned long len)
+static int do_brk_flags(unsigned long addr, unsigned long len, unsigned long 
flags)
 {
struct mm_struct *mm = current->mm;
struct vm_area_struct *vma, *prev;
-   unsigned long flags;
struct rb_node **rb_link, *rb_parent;
pgoff_t pgoff = addr >> PAGE_SHIFT;
int error;
@@ -2666,7 +2665,7 @@ static int do_brk(unsigned long addr, unsigned long len)
if (!len)
return 0;

-   flags = VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;
+   flags |= VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;


For sanity's sake, should a mask be applied here? i.e. to be extra
careful about what flags can get passed in?


Maybe... I am leaving it to mm experts.


Otherwise, this looks okay to me:

Reviewed-by: Kees Cook 

-Kees

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-09 Thread Michael Ellerman

Denys Vlasenko  writes:

> 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:
...
>
>  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| 34 ++
>  include/linux/mm.h |  1 +
>  mm/mmap.c  | 20 +++-
>  6 files changed, 43 insertions(+), 28 deletions(-)

What tree is this against?

I can't get it to apply to either Linus' tree or linux-next.

cheers

$ patch --dry-run -p1 < diff.diff
checking file arch/powerpc/include/asm/page.h
checking file arch/powerpc/include/asm/page_32.h
checking file arch/powerpc/include/asm/page_64.h
checking file fs/binfmt_elf.c
Hunk #3 FAILED at 613.
Hunk #4 FAILED at 633.
Hunk #5 succeeded at 681 (offset 2 lines).
Hunk #6 succeeded at 889 (offset 2 lines).
Hunk #7 succeeded at 984 (offset 2 lines).
Hunk #8 succeeded at 1003 (offset 2 lines).
2 out of 8 hunks FAILED
checking file include/linux/mm.h
checking file mm/mmap.c
Hunk #1 FAILED at 2653.
Hunk #2 succeeded at 2668 (offset 2 lines).
Hunk #3 succeeded at 2736 (offset 2 lines).
Hunk #4 succeeded at 2750 (offset 2 lines).
1 out of 4 hunks FAILED

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-09 Thread Michael Ellerman

Denys Vlasenko  writes:

> 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:
...
>
>  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| 34 ++
>  include/linux/mm.h |  1 +
>  mm/mmap.c  | 20 +++-
>  6 files changed, 43 insertions(+), 28 deletions(-)

What tree is this against?

I can't get it to apply to either Linus' tree or linux-next.

cheers

$ patch --dry-run -p1 < diff.diff
checking file arch/powerpc/include/asm/page.h
checking file arch/powerpc/include/asm/page_32.h
checking file arch/powerpc/include/asm/page_64.h
checking file fs/binfmt_elf.c
Hunk #3 FAILED at 613.
Hunk #4 FAILED at 633.
Hunk #5 succeeded at 681 (offset 2 lines).
Hunk #6 succeeded at 889 (offset 2 lines).
Hunk #7 succeeded at 984 (offset 2 lines).
Hunk #8 succeeded at 1003 (offset 2 lines).
2 out of 8 hunks FAILED
checking file include/linux/mm.h
checking file mm/mmap.c
Hunk #1 FAILED at 2653.
Hunk #2 succeeded at 2668 (offset 2 lines).
Hunk #3 succeeded at 2736 (offset 2 lines).
Hunk #4 succeeded at 2750 (offset 2 lines).
1 out of 4 hunks FAILED

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-09 Thread Kees Cook

On Tue, Aug 9, 2016 at 12:08 PM, Denys Vlasenko  wrote:
> On 32-bit powerps the ELF PLT sections of binaries (built with --bss-plt,

Typo: powerps -> powerpc

> or with a toolchain which defaults to it) look like this:
>
>   [17] .sbss NOBITS  0002aff8 01aff8 14 00  WA  0   0 
>  4
>   [18] .plt  NOBITS  0002b00c 01aff8 84 00 WAX  0   0 
>  4
>   [19] .bss  NOBITS  0002b090 01aff8 a4 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 0x1
>
> 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 
> Signed-off-by: Denys Vlasenko 
> CC: Benjamin Herrenschmidt 
> CC: Paul Mackerras 
> CC: Kees Cook 
> CC: Oleg Nesterov 
> CC: Michael Ellerman 
> CC: Florian Weimer 
> CC: linux...@kvack.org
> CC: linuxppc-...@lists.ozlabs.org
> CC: linux-kernel@vger.kernel.org
> ---
> Changes since v2:
> * moved capability to map with VM_EXEC into vm_brk_flags()
>
> 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| 34 ++
>  include/linux/mm.h |  1 +
>  mm/mmap.c  | 20 +++-
>  6 files changed, 43 insertions(+), 28 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..d4a1966 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -91,12 +91,18 @@ static struct linux_binfmt elf_format = {
>
>  #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
>
> -static int set_brk(unsigned long start, unsigned long

Re: [PATCH v3] powerpc: Do not make the entire heap executable

2016-08-09 Thread Kees Cook

On Tue, Aug 9, 2016 at 12:08 PM, Denys Vlasenko  wrote:
> On 32-bit powerps the ELF PLT sections of binaries (built with --bss-plt,

Typo: powerps -> powerpc

> or with a toolchain which defaults to it) look like this:
>
>   [17] .sbss NOBITS  0002aff8 01aff8 14 00  WA  0   0 
>  4
>   [18] .plt  NOBITS  0002b00c 01aff8 84 00 WAX  0   0 
>  4
>   [19] .bss  NOBITS  0002b090 01aff8 a4 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 0x1
>
> 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 
> Signed-off-by: Denys Vlasenko 
> CC: Benjamin Herrenschmidt 
> CC: Paul Mackerras 
> CC: Kees Cook 
> CC: Oleg Nesterov 
> CC: Michael Ellerman 
> CC: Florian Weimer 
> CC: linux...@kvack.org
> CC: linuxppc-...@lists.ozlabs.org
> CC: linux-kernel@vger.kernel.org
> ---
> Changes since v2:
> * moved capability to map with VM_EXEC into vm_brk_flags()
>
> 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| 34 ++
>  include/linux/mm.h |  1 +
>  mm/mmap.c  | 20 +++-
>  6 files changed, 43 insertions(+), 28 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..d4a1966 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -91,12 +91,18 @@ 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) {
> -

[PATCH v3] powerpc: Do not make the entire heap executable

2016-08-09 Thread Denys Vlasenko

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 14 00  WA  0   0  4
  [18] .plt  NOBITS  0002b00c 01aff8 84 00 WAX  0   0  4
  [19] .bss  NOBITS  0002b090 01aff8 a4 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 0x1

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 
Signed-off-by: Denys Vlasenko 
CC: Benjamin Herrenschmidt 
CC: Paul Mackerras 
CC: Kees Cook 
CC: Oleg Nesterov 
CC: Michael Ellerman 
CC: Florian Weimer 
CC: linux...@kvack.org
CC: linuxppc-...@lists.ozlabs.org
CC: linux-kernel@vger.kernel.org
---
Changes since v2:
* moved capability to map with VM_EXEC into vm_brk_flags()

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| 34 ++
 include/linux/mm.h |  1 +
 mm/mmap.c  | 20 +++-
 6 files changed, 43 insertions(+), 28 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..d4a1966 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -91,12 +91,18 @@ 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);
+   /*
+* Map the last of the bss segment.
+* If the header is

[PATCH v3] powerpc: Do not make the entire heap executable

2016-08-09 Thread Denys Vlasenko

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 14 00  WA  0   0  4
  [18] .plt  NOBITS  0002b00c 01aff8 84 00 WAX  0   0  4
  [19] .bss  NOBITS  0002b090 01aff8 a4 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 0x1

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 
Signed-off-by: Denys Vlasenko 
CC: Benjamin Herrenschmidt 
CC: Paul Mackerras 
CC: Kees Cook 
CC: Oleg Nesterov 
CC: Michael Ellerman 
CC: Florian Weimer 
CC: linux...@kvack.org
CC: linuxppc-...@lists.ozlabs.org
CC: linux-kernel@vger.kernel.org
---
Changes since v2:
* moved capability to map with VM_EXEC into vm_brk_flags()

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| 34 ++
 include/linux/mm.h |  1 +
 mm/mmap.c  | 20 +++-
 6 files changed, 43 insertions(+), 28 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..d4a1966 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -91,12 +91,18 @@ 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);
+   /*
+* Map the last of the bss segment.
+* If the header is requesting these pages to be
+* executable, honour that (ppc32 needs this).
+*/
+   int error = vm_brk_flags(start, end - start,
+

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

Re: [PATCH v3] powerpc: Do not make the entire heap executable

[PATCH v3] powerpc: Do not make the entire heap executable

[PATCH v3] powerpc: Do not make the entire heap executable

10 matches

Site Navigation

Mail list logo

Footer information