All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] handle worst-case heap randomization in mmap_base
@ 2019-03-12 17:32 ` Ali Saidi
  0 siblings, 0 replies; 26+ messages in thread
From: Ali Saidi @ 2019-03-12 17:32 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, x86
  Cc: H. Peter Anvin, Andrew Morton, Ali Saidi, Kees Cook,
	Borislav Petkov, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
	Andy Lutomirski, Dave Hansen, Will Deacon, Catalin Marinas,
	David Woodhouse, Anthony Liguori

Increase mmap_base by the worst-case brk randomization so that the stack and
heap remain apart.

In Linux 4.13 a change was committed that special cased the kernel ELF
loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
directly and this issue is limited to cases where it is, (e.g to set a
non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
those rare cases, the loader doesn't take into account the amount of brk
randomization that will be applied by arch_randomize_brk(). This can
lead to the stack and heap being arbitrarily close to each other.

Ali Saidi (2):
  arm64/mmap: handle worst-case heap randomization in mmap_base
  x86/mmap: handle worst-case heap randomization in mmap_base

 arch/arm64/mm/mmap.c | 8 ++++++++
 arch/x86/mm/mmap.c   | 4 ++++
 2 files changed, 12 insertions(+)

-- 
2.15.3.AMZN


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 0/2] handle worst-case heap randomization in mmap_base
@ 2019-03-12 17:32 ` Ali Saidi
  0 siblings, 0 replies; 26+ messages in thread
From: Ali Saidi @ 2019-03-12 17:32 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, x86
  Cc: Kees Cook, Peter Zijlstra, Catalin Marinas, Dave Hansen,
	Will Deacon, Ingo Molnar, Borislav Petkov, David Woodhouse,
	Andy Lutomirski, H. Peter Anvin, Andrew Morton, Thomas Gleixner,
	Ali Saidi, Anthony Liguori

Increase mmap_base by the worst-case brk randomization so that the stack and
heap remain apart.

In Linux 4.13 a change was committed that special cased the kernel ELF
loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
directly and this issue is limited to cases where it is, (e.g to set a
non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
those rare cases, the loader doesn't take into account the amount of brk
randomization that will be applied by arch_randomize_brk(). This can
lead to the stack and heap being arbitrarily close to each other.

Ali Saidi (2):
  arm64/mmap: handle worst-case heap randomization in mmap_base
  x86/mmap: handle worst-case heap randomization in mmap_base

 arch/arm64/mm/mmap.c | 8 ++++++++
 arch/x86/mm/mmap.c   | 4 ++++
 2 files changed, 12 insertions(+)

-- 
2.15.3.AMZN


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/2] arm64/mmap: handle worst-case heap randomization in mmap_base
  2019-03-12 17:32 ` Ali Saidi
@ 2019-03-12 17:32   ` Ali Saidi
  -1 siblings, 0 replies; 26+ messages in thread
From: Ali Saidi @ 2019-03-12 17:32 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, x86
  Cc: H. Peter Anvin, Andrew Morton, Ali Saidi, Kees Cook,
	Borislav Petkov, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
	Andy Lutomirski, Dave Hansen, Will Deacon, Catalin Marinas,
	David Woodhouse, Anthony Liguori

Increase mmap_base by the worst-case brk randomization so that
the stack and heap remain apart.

In Linux 4.13 a change was committed that special cased the kernel ELF
loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
directly and this issue is limited to cases where it is, (e.g to set a
non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
those rare cases, the loader doesn't take into account the amount of brk
randomization that will be applied by arch_randomize_brk(). This can
lead to the stack and heap being arbitrarily close to each other.

Signed-off-by: Ali Saidi <alisaidi@amazon.com>

---
 arch/arm64/mm/mmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index 842c8a5fcd53..0778f7ba8306 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -67,6 +67,14 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
 	unsigned long gap = rlim_stack->rlim_cur;
 	unsigned long pad = (STACK_RND_MASK << PAGE_SHIFT) + stack_guard_gap;
 
+	/* Provide space for randomization when randomize_va_space == 2 and
+	 * ld-linux.so is called directly. Values from arch_randomize_brk()
+	 */
+	if (test_thread_flag(TIF_32BIT))
+		pad += SZ_32M;
+	else
+		pad += SZ_1G;
+
 	/* Values close to RLIM_INFINITY can overflow. */
 	if (gap + pad > gap)
 		gap += pad;
-- 
2.15.3.AMZN


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 1/2] arm64/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-12 17:32   ` Ali Saidi
  0 siblings, 0 replies; 26+ messages in thread
From: Ali Saidi @ 2019-03-12 17:32 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, x86
  Cc: Kees Cook, Peter Zijlstra, Catalin Marinas, Dave Hansen,
	Will Deacon, Ingo Molnar, Borislav Petkov, David Woodhouse,
	Andy Lutomirski, H. Peter Anvin, Andrew Morton, Thomas Gleixner,
	Ali Saidi, Anthony Liguori

Increase mmap_base by the worst-case brk randomization so that
the stack and heap remain apart.

In Linux 4.13 a change was committed that special cased the kernel ELF
loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
directly and this issue is limited to cases where it is, (e.g to set a
non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
those rare cases, the loader doesn't take into account the amount of brk
randomization that will be applied by arch_randomize_brk(). This can
lead to the stack and heap being arbitrarily close to each other.

Signed-off-by: Ali Saidi <alisaidi@amazon.com>

---
 arch/arm64/mm/mmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index 842c8a5fcd53..0778f7ba8306 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -67,6 +67,14 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
 	unsigned long gap = rlim_stack->rlim_cur;
 	unsigned long pad = (STACK_RND_MASK << PAGE_SHIFT) + stack_guard_gap;
 
+	/* Provide space for randomization when randomize_va_space == 2 and
+	 * ld-linux.so is called directly. Values from arch_randomize_brk()
+	 */
+	if (test_thread_flag(TIF_32BIT))
+		pad += SZ_32M;
+	else
+		pad += SZ_1G;
+
 	/* Values close to RLIM_INFINITY can overflow. */
 	if (gap + pad > gap)
 		gap += pad;
-- 
2.15.3.AMZN


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-12 17:32 ` Ali Saidi
@ 2019-03-12 17:32   ` Ali Saidi
  -1 siblings, 0 replies; 26+ messages in thread
From: Ali Saidi @ 2019-03-12 17:32 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, x86
  Cc: H. Peter Anvin, Andrew Morton, Ali Saidi, Kees Cook,
	Borislav Petkov, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
	Andy Lutomirski, Dave Hansen, Will Deacon, Catalin Marinas,
	David Woodhouse, Anthony Liguori

Increase mmap_base by the worst-case brk randomization so that
the stack and heap remain apart.

In Linux 4.13 a change was committed that special cased the kernel ELF
loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
directly and this issue is limited to cases where it is, (e.g to set a
non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
those rare cases, the loader doesn't take into account the amount of brk
randomization that will be applied by arch_randomize_brk(). This can
lead to the stack and heap being arbitrarily close to each other.

Signed-off-by: Ali Saidi <alisaidi@amazon.com>
---
 arch/x86/mm/mmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index db3165714521..98a2875c37e3 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,7 @@
 #include <linux/sched/signal.h>
 #include <linux/sched/mm.h>
 #include <linux/compat.h>
+#include <linux/sizes.h>
 #include <asm/elf.h>
 
 #include "physaddr.h"
@@ -97,6 +98,9 @@ static unsigned long mmap_base(unsigned long rnd, unsigned long task_size,
 	unsigned long pad = stack_maxrandom_size(task_size) + stack_guard_gap;
 	unsigned long gap_min, gap_max;
 
+	/* Provide space for brk randomization */
+	pad += SZ_32M;
+
 	/* Values close to RLIM_INFINITY can overflow. */
 	if (gap + pad > gap)
 		gap += pad;
-- 
2.15.3.AMZN


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-12 17:32   ` Ali Saidi
  0 siblings, 0 replies; 26+ messages in thread
From: Ali Saidi @ 2019-03-12 17:32 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, x86
  Cc: Kees Cook, Peter Zijlstra, Catalin Marinas, Dave Hansen,
	Will Deacon, Ingo Molnar, Borislav Petkov, David Woodhouse,
	Andy Lutomirski, H. Peter Anvin, Andrew Morton, Thomas Gleixner,
	Ali Saidi, Anthony Liguori

Increase mmap_base by the worst-case brk randomization so that
the stack and heap remain apart.

In Linux 4.13 a change was committed that special cased the kernel ELF
loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
directly and this issue is limited to cases where it is, (e.g to set a
non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
those rare cases, the loader doesn't take into account the amount of brk
randomization that will be applied by arch_randomize_brk(). This can
lead to the stack and heap being arbitrarily close to each other.

Signed-off-by: Ali Saidi <alisaidi@amazon.com>
---
 arch/x86/mm/mmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index db3165714521..98a2875c37e3 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,7 @@
 #include <linux/sched/signal.h>
 #include <linux/sched/mm.h>
 #include <linux/compat.h>
+#include <linux/sizes.h>
 #include <asm/elf.h>
 
 #include "physaddr.h"
@@ -97,6 +98,9 @@ static unsigned long mmap_base(unsigned long rnd, unsigned long task_size,
 	unsigned long pad = stack_maxrandom_size(task_size) + stack_guard_gap;
 	unsigned long gap_min, gap_max;
 
+	/* Provide space for brk randomization */
+	pad += SZ_32M;
+
 	/* Values close to RLIM_INFINITY can overflow. */
 	if (gap + pad > gap)
 		gap += pad;
-- 
2.15.3.AMZN


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-12 17:32   ` Ali Saidi
@ 2019-03-13 16:25     ` Dave Hansen
  -1 siblings, 0 replies; 26+ messages in thread
From: Dave Hansen @ 2019-03-13 16:25 UTC (permalink / raw)
  To: Ali Saidi, linux-kernel, linux-arm-kernel, x86
  Cc: H. Peter Anvin, Andrew Morton, Kees Cook, Borislav Petkov,
	Ingo Molnar, Thomas Gleixner, Peter Zijlstra, Andy Lutomirski,
	Dave Hansen, Will Deacon, Catalin Marinas, David Woodhouse,
	Anthony Liguori

On 3/12/19 10:32 AM, Ali Saidi wrote:
> +	/* Provide space for brk randomization */
> +	pad += SZ_32M;

Just curious:  Why is the padding in your other patch conditional on the
32-bit vs. 64-bit apps, but here it's always 32M?

Also, did you hit this problem in practice somehow?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-13 16:25     ` Dave Hansen
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Hansen @ 2019-03-13 16:25 UTC (permalink / raw)
  To: Ali Saidi, linux-kernel, linux-arm-kernel, x86
  Cc: Kees Cook, Peter Zijlstra, Catalin Marinas, Dave Hansen,
	Will Deacon, Ingo Molnar, Borislav Petkov, Andy Lutomirski,
	H. Peter Anvin, Andrew Morton, Thomas Gleixner, David Woodhouse,
	Anthony Liguori

On 3/12/19 10:32 AM, Ali Saidi wrote:
> +	/* Provide space for brk randomization */
> +	pad += SZ_32M;

Just curious:  Why is the padding in your other patch conditional on the
32-bit vs. 64-bit apps, but here it's always 32M?

Also, did you hit this problem in practice somehow?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-12 17:32   ` Ali Saidi
@ 2019-03-13 22:58     ` Kees Cook
  -1 siblings, 0 replies; 26+ messages in thread
From: Kees Cook @ 2019-03-13 22:58 UTC (permalink / raw)
  To: Ali Saidi
  Cc: LKML, linux-arm-kernel, X86 ML, H. Peter Anvin, Andrew Morton,
	Borislav Petkov, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
	Andy Lutomirski, Dave Hansen, Will Deacon, Catalin Marinas,
	David Woodhouse, Anthony Liguori

On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi <alisaidi@amazon.com> wrote:
>
> Increase mmap_base by the worst-case brk randomization so that
> the stack and heap remain apart.
>
> In Linux 4.13 a change was committed that special cased the kernel ELF
> loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> directly and this issue is limited to cases where it is, (e.g to set a
> non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> those rare cases, the loader doesn't take into account the amount of brk
> randomization that will be applied by arch_randomize_brk(). This can
> lead to the stack and heap being arbitrarily close to each other.

In the case of using the loader directly, brk (so helpfully identified
as "[heap]") is allocated with the _loader_ not the binary. For
example, with ASLR entirely disabled, you can see this more clearly:

$ /bin/cat /proc/self/maps
555555554000-55555555c000 r-xp 00000000 fd:02 34603015
  /bin/cat
55555575b000-55555575c000 r--p 00007000 fd:02 34603015
  /bin/cat
55555575c000-55555575d000 rw-p 00008000 fd:02 34603015
  /bin/cat
55555575d000-55555577e000 rw-p 00000000 00:00 0                          [heap]
...
7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]

$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
...
7ffff7bcc000-7ffff7bd4000 r-xp 00000000 fd:02 34603015
  /bin/cat
7ffff7bd4000-7ffff7dd3000 ---p 00008000 fd:02 34603015
  /bin/cat
7ffff7dd3000-7ffff7dd4000 r--p 00007000 fd:02 34603015
  /bin/cat
7ffff7dd4000-7ffff7dd5000 rw-p 00008000 fd:02 34603015
  /bin/cat
7ffff7dd5000-7ffff7dfc000 r-xp 00000000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7fb2000-7ffff7fd6000 rw-p 00000000 00:00 0
7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffe000-7ffff8020000 rw-p 00000000 00:00 0                          [heap]
7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]

So I think changing this globally isn't the right solution (normally
brk is between text and mmap). Adjusting the mmap base padding means
we lose even more memory space. Perhaps it would be better if brk
allocation would be placed before the mmap region (i.e. use
ELF_ET_DYN_BASE). This seems to work for me:

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 7d09d125f148..cdaa33f4a3ef 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1131,6 +1131,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
        current->mm->end_data = end_data;
        current->mm->start_stack = bprm->p;

+       /*
+        * When executing a loader directly (ET_DYN without Interp), move
+        * the brk area out of the mmap region (since it grows up, and may
+        * collide early with the stack growing down), and into the unused
+        * ELF_ET_DYN_BASE region.
+        */
+       if (!elf_interpreter)
+               current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
+
        if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
                current->mm->brk = current->mm->start_brk =
                        arch_randomize_brk(current->mm);

$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
555556de3000-555556e04000 rw-p 00000000 00:00 0                          [heap]
7f8467da9000-7f8467f90000 r-xp 00000000 fd:01 399295
  /lib/x86_64-linux-gnu/libc-2.27.so
...
7f846819a000-7f84681a2000 r-xp 00000000 fd:01 263229
  /bin/cat
...
7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
  /lib/x86_64-linux-gnu/ld-2.27.so
7f84685cc000-7f84685cd000 rw-p 00000000 00:00 0
7ffce68f8000-7ffce6919000 rw-p 00000000 00:00 0                          [stack]
7ffce69f0000-7ffce69f3000 r--p 00000000 00:00 0                          [vvar]
7ffce69f3000-7ffce69f4000 r-xp 00000000 00:00 0                          [vdso]

Does anyone see problems with this? (Note that ET_EXEC base is
0x400000, so no collision there...)

-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-13 22:58     ` Kees Cook
  0 siblings, 0 replies; 26+ messages in thread
From: Kees Cook @ 2019-03-13 22:58 UTC (permalink / raw)
  To: Ali Saidi
  Cc: Dave Hansen, Anthony Liguori, Peter Zijlstra, Catalin Marinas,
	X86 ML, Will Deacon, LKML, Ingo Molnar, Borislav Petkov,
	David Woodhouse, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Thomas Gleixner, linux-arm-kernel

On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi <alisaidi@amazon.com> wrote:
>
> Increase mmap_base by the worst-case brk randomization so that
> the stack and heap remain apart.
>
> In Linux 4.13 a change was committed that special cased the kernel ELF
> loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> directly and this issue is limited to cases where it is, (e.g to set a
> non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> those rare cases, the loader doesn't take into account the amount of brk
> randomization that will be applied by arch_randomize_brk(). This can
> lead to the stack and heap being arbitrarily close to each other.

In the case of using the loader directly, brk (so helpfully identified
as "[heap]") is allocated with the _loader_ not the binary. For
example, with ASLR entirely disabled, you can see this more clearly:

$ /bin/cat /proc/self/maps
555555554000-55555555c000 r-xp 00000000 fd:02 34603015
  /bin/cat
55555575b000-55555575c000 r--p 00007000 fd:02 34603015
  /bin/cat
55555575c000-55555575d000 rw-p 00008000 fd:02 34603015
  /bin/cat
55555575d000-55555577e000 rw-p 00000000 00:00 0                          [heap]
...
7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]

$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
...
7ffff7bcc000-7ffff7bd4000 r-xp 00000000 fd:02 34603015
  /bin/cat
7ffff7bd4000-7ffff7dd3000 ---p 00008000 fd:02 34603015
  /bin/cat
7ffff7dd3000-7ffff7dd4000 r--p 00007000 fd:02 34603015
  /bin/cat
7ffff7dd4000-7ffff7dd5000 rw-p 00008000 fd:02 34603015
  /bin/cat
7ffff7dd5000-7ffff7dfc000 r-xp 00000000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7fb2000-7ffff7fd6000 rw-p 00000000 00:00 0
7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
  /lib/x86_64-linux-gnu/ld-2.27.so
7ffff7ffe000-7ffff8020000 rw-p 00000000 00:00 0                          [heap]
7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]

So I think changing this globally isn't the right solution (normally
brk is between text and mmap). Adjusting the mmap base padding means
we lose even more memory space. Perhaps it would be better if brk
allocation would be placed before the mmap region (i.e. use
ELF_ET_DYN_BASE). This seems to work for me:

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 7d09d125f148..cdaa33f4a3ef 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1131,6 +1131,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
        current->mm->end_data = end_data;
        current->mm->start_stack = bprm->p;

+       /*
+        * When executing a loader directly (ET_DYN without Interp), move
+        * the brk area out of the mmap region (since it grows up, and may
+        * collide early with the stack growing down), and into the unused
+        * ELF_ET_DYN_BASE region.
+        */
+       if (!elf_interpreter)
+               current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
+
        if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
                current->mm->brk = current->mm->start_brk =
                        arch_randomize_brk(current->mm);

$ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
555556de3000-555556e04000 rw-p 00000000 00:00 0                          [heap]
7f8467da9000-7f8467f90000 r-xp 00000000 fd:01 399295
  /lib/x86_64-linux-gnu/libc-2.27.so
...
7f846819a000-7f84681a2000 r-xp 00000000 fd:01 263229
  /bin/cat
...
7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
  /lib/x86_64-linux-gnu/ld-2.27.so
7f84685cc000-7f84685cd000 rw-p 00000000 00:00 0
7ffce68f8000-7ffce6919000 rw-p 00000000 00:00 0                          [stack]
7ffce69f0000-7ffce69f3000 r--p 00000000 00:00 0                          [vvar]
7ffce69f3000-7ffce69f4000 r-xp 00000000 00:00 0                          [vdso]

Does anyone see problems with this? (Note that ET_EXEC base is
0x400000, so no collision there...)

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-13 16:25     ` Dave Hansen
@ 2019-03-17 15:52       ` Saidi, Ali
  -1 siblings, 0 replies; 26+ messages in thread
From: Saidi, Ali @ 2019-03-17 15:52 UTC (permalink / raw)
  To: Dave Hansen
  Cc: linux-kernel, linux-arm-kernel, x86, H. Peter Anvin,
	Andrew Morton, Kees Cook, Borislav Petkov, Ingo Molnar,
	Thomas Gleixner, Peter Zijlstra, Andy Lutomirski, Dave Hansen,
	Will Deacon, Catalin Marinas, Woodhouse, David, Liguori, Anthony



> On Mar 13, 2019, at 11:26 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
>> On 3/12/19 10:32 AM, Ali Saidi wrote:
>> +    /* Provide space for brk randomization */
>> +    pad += SZ_32M;
> 
> Just curious:  Why is the padding in your other patch conditional on the
> 32-bit vs. 64-bit apps, but here it's always 32M?
Arm changes the amount of brk based on the process being 32 vs 64 bit. X86 doesn’t appear to do this. 
> 
> Also, did you hit this problem in practice somehow?

Just debugging a crash when testing a version of a library I compiled. 

Ali

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-17 15:52       ` Saidi, Ali
  0 siblings, 0 replies; 26+ messages in thread
From: Saidi, Ali @ 2019-03-17 15:52 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Dave Hansen, Kees Cook, Peter Zijlstra, Catalin Marinas, x86,
	Will Deacon, linux-kernel, Ingo Molnar, Borislav Petkov,
	Woodhouse, David, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Thomas Gleixner, linux-arm-kernel, Liguori, Anthony



> On Mar 13, 2019, at 11:26 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
>> On 3/12/19 10:32 AM, Ali Saidi wrote:
>> +    /* Provide space for brk randomization */
>> +    pad += SZ_32M;
> 
> Just curious:  Why is the padding in your other patch conditional on the
> 32-bit vs. 64-bit apps, but here it's always 32M?
Arm changes the amount of brk based on the process being 32 vs 64 bit. X86 doesn’t appear to do this. 
> 
> Also, did you hit this problem in practice somehow?

Just debugging a crash when testing a version of a library I compiled. 

Ali
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-12 17:32   ` Ali Saidi
@ 2019-03-21 14:09     ` Thomas Gleixner
  -1 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2019-03-21 14:09 UTC (permalink / raw)
  To: Ali Saidi
  Cc: linux-kernel, linux-arm-kernel, x86, H. Peter Anvin,
	Andrew Morton, Kees Cook, Borislav Petkov, Ingo Molnar,
	Peter Zijlstra, Andy Lutomirski, Dave Hansen, Will Deacon,
	Catalin Marinas, David Woodhouse, Anthony Liguori

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

On Tue, 12 Mar 2019, Ali Saidi wrote:

> Increase mmap_base by the worst-case brk randomization so that
> the stack and heap remain apart.
> 
> In Linux 4.13 a change was committed that special cased the kernel ELF
> loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> directly and this issue is limited to cases where it is, (e.g to set a
> non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> those rare cases, the loader doesn't take into account the amount of brk
> randomization that will be applied by arch_randomize_brk(). This can
> lead to the stack and heap being arbitrarily close to each other.

That explains not why you need this change. What's the consequence of them
being close to each other?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-21 14:09     ` Thomas Gleixner
  0 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2019-03-21 14:09 UTC (permalink / raw)
  To: Ali Saidi
  Cc: Dave Hansen, Kees Cook, Peter Zijlstra, Catalin Marinas, x86,
	Will Deacon, linux-kernel, Ingo Molnar, Borislav Petkov,
	David Woodhouse, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	linux-arm-kernel, Anthony Liguori

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

On Tue, 12 Mar 2019, Ali Saidi wrote:

> Increase mmap_base by the worst-case brk randomization so that
> the stack and heap remain apart.
> 
> In Linux 4.13 a change was committed that special cased the kernel ELF
> loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> directly and this issue is limited to cases where it is, (e.g to set a
> non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> those rare cases, the loader doesn't take into account the amount of brk
> randomization that will be applied by arch_randomize_brk(). This can
> lead to the stack and heap being arbitrarily close to each other.

That explains not why you need this change. What's the consequence of them
being close to each other?

Thanks,

	tglx

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-21 14:09     ` Thomas Gleixner
@ 2019-03-26  2:13       ` Saidi, Ali
  -1 siblings, 0 replies; 26+ messages in thread
From: Saidi, Ali @ 2019-03-26  2:13 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Dave Hansen, Kees Cook, Peter Zijlstra, Catalin Marinas, x86,
	Will Deacon, linux-kernel, Ingo Molnar, Borislav Petkov,
	Woodhouse, David, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	linux-arm-kernel, Liguori, Anthony



On 3/21/19, 9:11 AM, "linux-arm-kernel on behalf of Thomas Gleixner" <linux-arm-kernel-bounces@lists.infradead.org on behalf of tglx@linutronix.de> wrote:

    On Tue, 12 Mar 2019, Ali Saidi wrote:
    
    > Increase mmap_base by the worst-case brk randomization so that
    > the stack and heap remain apart.
    > 
    > In Linux 4.13 a change was committed that special cased the kernel ELF
    > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
    > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
    > directly and this issue is limited to cases where it is, (e.g to set a
    > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
    > those rare cases, the loader doesn't take into account the amount of brk
    > randomization that will be applied by arch_randomize_brk(). This can
    > lead to the stack and heap being arbitrarily close to each other.
    
    That explains not why you need this change. What's the consequence of them
    being close to each other?
    
The process doesn't get it's requested stack size and stack allocations could end up scribbling on the heap.

Thanks,
Ali
 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-26  2:13       ` Saidi, Ali
  0 siblings, 0 replies; 26+ messages in thread
From: Saidi, Ali @ 2019-03-26  2:13 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Dave Hansen, Kees Cook, Peter Zijlstra, Catalin Marinas, x86,
	Will Deacon, linux-kernel, Ingo Molnar, Borislav Petkov,
	linux-arm-kernel, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Woodhouse, David, Liguori, Anthony



On 3/21/19, 9:11 AM, "linux-arm-kernel on behalf of Thomas Gleixner" <linux-arm-kernel-bounces@lists.infradead.org on behalf of tglx@linutronix.de> wrote:

    On Tue, 12 Mar 2019, Ali Saidi wrote:
    
    > Increase mmap_base by the worst-case brk randomization so that
    > the stack and heap remain apart.
    > 
    > In Linux 4.13 a change was committed that special cased the kernel ELF
    > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
    > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
    > directly and this issue is limited to cases where it is, (e.g to set a
    > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
    > those rare cases, the loader doesn't take into account the amount of brk
    > randomization that will be applied by arch_randomize_brk(). This can
    > lead to the stack and heap being arbitrarily close to each other.
    
    That explains not why you need this change. What's the consequence of them
    being close to each other?
    
The process doesn't get it's requested stack size and stack allocations could end up scribbling on the heap.

Thanks,
Ali
 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-26  2:13       ` Saidi, Ali
@ 2019-03-26  8:43         ` Thomas Gleixner
  -1 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2019-03-26  8:43 UTC (permalink / raw)
  To: Saidi, Ali
  Cc: Dave Hansen, Kees Cook, Peter Zijlstra, Catalin Marinas, x86,
	Will Deacon, linux-kernel, Ingo Molnar, Borislav Petkov,
	Woodhouse, David, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	linux-arm-kernel, Liguori, Anthony

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Tue, 26 Mar 2019, Saidi, Ali wrote:
> On 3/21/19, 9:11 AM, "linux-arm-kernel on behalf of Thomas Gleixner" <linux-arm-kernel-bounces@lists.infradead.org on behalf of tglx@linutronix.de> wrote:
> 
>     On Tue, 12 Mar 2019, Ali Saidi wrote:
>     
>     > Increase mmap_base by the worst-case brk randomization so that
>     > the stack and heap remain apart.
>     > 
>     > In Linux 4.13 a change was committed that special cased the kernel ELF
>     > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
>     > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
>     > directly and this issue is limited to cases where it is, (e.g to set a
>     > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
>     > those rare cases, the loader doesn't take into account the amount of brk
>     > randomization that will be applied by arch_randomize_brk(). This can
>     > lead to the stack and heap being arbitrarily close to each other.
>     
>     That explains not why you need this change. What's the consequence of them
>     being close to each other?
>
> The process doesn't get it's requested stack size and stack allocations
> could end up scribbling on the heap.

And exactly that information wants to be in the changelog.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-26  8:43         ` Thomas Gleixner
  0 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2019-03-26  8:43 UTC (permalink / raw)
  To: Saidi, Ali
  Cc: Dave Hansen, Kees Cook, Peter Zijlstra, Catalin Marinas, x86,
	Will Deacon, linux-kernel, Ingo Molnar, Borislav Petkov,
	linux-arm-kernel, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Woodhouse, David, Liguori, Anthony

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Tue, 26 Mar 2019, Saidi, Ali wrote:
> On 3/21/19, 9:11 AM, "linux-arm-kernel on behalf of Thomas Gleixner" <linux-arm-kernel-bounces@lists.infradead.org on behalf of tglx@linutronix.de> wrote:
> 
>     On Tue, 12 Mar 2019, Ali Saidi wrote:
>     
>     > Increase mmap_base by the worst-case brk randomization so that
>     > the stack and heap remain apart.
>     > 
>     > In Linux 4.13 a change was committed that special cased the kernel ELF
>     > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
>     > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
>     > directly and this issue is limited to cases where it is, (e.g to set a
>     > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
>     > those rare cases, the loader doesn't take into account the amount of brk
>     > randomization that will be applied by arch_randomize_brk(). This can
>     > lead to the stack and heap being arbitrarily close to each other.
>     
>     That explains not why you need this change. What's the consequence of them
>     being close to each other?
>
> The process doesn't get it's requested stack size and stack allocations
> could end up scribbling on the heap.

And exactly that information wants to be in the changelog.

Thanks,

	tglx

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-13 22:58     ` Kees Cook
@ 2019-03-27 19:51       ` Kees Cook
  -1 siblings, 0 replies; 26+ messages in thread
From: Kees Cook @ 2019-03-27 19:51 UTC (permalink / raw)
  To: Ali Saidi, Michal Hocko, Matthew Wilcox, Jann Horn
  Cc: LKML, linux-arm-kernel, X86 ML, H. Peter Anvin, Andrew Morton,
	Borislav Petkov, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
	Andy Lutomirski, Dave Hansen, Will Deacon, Catalin Marinas,
	David Woodhouse, Anthony Liguori

On Wed, Mar 13, 2019 at 3:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi <alisaidi@amazon.com> wrote:
> >
> > Increase mmap_base by the worst-case brk randomization so that
> > the stack and heap remain apart.
> >
> > In Linux 4.13 a change was committed that special cased the kernel ELF
> > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> > directly and this issue is limited to cases where it is, (e.g to set a
> > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> > those rare cases, the loader doesn't take into account the amount of brk
> > randomization that will be applied by arch_randomize_brk(). This can
> > lead to the stack and heap being arbitrarily close to each other.
>
> In the case of using the loader directly, brk (so helpfully identified
> as "[heap]") is allocated with the _loader_ not the binary. For
> example, with ASLR entirely disabled, you can see this more clearly:
>
> $ /bin/cat /proc/self/maps
> 555555554000-55555555c000 r-xp 00000000 fd:02 34603015
>   /bin/cat
> 55555575b000-55555575c000 r--p 00007000 fd:02 34603015
>   /bin/cat
> 55555575c000-55555575d000 rw-p 00008000 fd:02 34603015
>   /bin/cat
> 55555575d000-55555577e000 rw-p 00000000 00:00 0                          [heap]
> ...
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
> 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> ...
> 7ffff7bcc000-7ffff7bd4000 r-xp 00000000 fd:02 34603015
>   /bin/cat
> 7ffff7bd4000-7ffff7dd3000 ---p 00008000 fd:02 34603015
>   /bin/cat
> 7ffff7dd3000-7ffff7dd4000 r--p 00007000 fd:02 34603015
>   /bin/cat
> 7ffff7dd4000-7ffff7dd5000 rw-p 00008000 fd:02 34603015
>   /bin/cat
> 7ffff7dd5000-7ffff7dfc000 r-xp 00000000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7fb2000-7ffff7fd6000 rw-p 00000000 00:00 0
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff8020000 rw-p 00000000 00:00 0                          [heap]
> 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
>
> So I think changing this globally isn't the right solution (normally
> brk is between text and mmap). Adjusting the mmap base padding means
> we lose even more memory space. Perhaps it would be better if brk
> allocation would be placed before the mmap region (i.e. use
> ELF_ET_DYN_BASE). This seems to work for me:
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 7d09d125f148..cdaa33f4a3ef 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1131,6 +1131,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
>         current->mm->end_data = end_data;
>         current->mm->start_stack = bprm->p;
>
> +       /*
> +        * When executing a loader directly (ET_DYN without Interp), move
> +        * the brk area out of the mmap region (since it grows up, and may
> +        * collide early with the stack growing down), and into the unused
> +        * ELF_ET_DYN_BASE region.
> +        */
> +       if (!elf_interpreter)
> +               current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
> +
>         if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
>                 current->mm->brk = current->mm->start_brk =
>                         arch_randomize_brk(current->mm);
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> 555556de3000-555556e04000 rw-p 00000000 00:00 0                          [heap]
> 7f8467da9000-7f8467f90000 r-xp 00000000 fd:01 399295
>   /lib/x86_64-linux-gnu/libc-2.27.so
> ...
> 7f846819a000-7f84681a2000 r-xp 00000000 fd:01 263229
>   /bin/cat
> ...
> 7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7f84685cc000-7f84685cd000 rw-p 00000000 00:00 0
> 7ffce68f8000-7ffce6919000 rw-p 00000000 00:00 0                          [stack]
> 7ffce69f0000-7ffce69f3000 r--p 00000000 00:00 0                          [vvar]
> 7ffce69f3000-7ffce69f4000 r-xp 00000000 00:00 0                          [vdso]
>
> Does anyone see problems with this? (Note that ET_EXEC base is
> 0x400000, so no collision there...)

Adding some more people to CC... what do people think about this
moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
that worked before should still work (i.e. the ultimately-launched
binary already had the brk very far from its text, so this should be
no different from a COMPAT_BRK standpoint). The only risk I see here
is that if someone started to suddenly depend on the entire memory
space above the mmap region being available when launching binaries
via a direct loader execs... which seems highly unlikely, I'd hope:
this would mean a binary would not work when execed normally.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-03-27 19:51       ` Kees Cook
  0 siblings, 0 replies; 26+ messages in thread
From: Kees Cook @ 2019-03-27 19:51 UTC (permalink / raw)
  To: Ali Saidi, Michal Hocko, Matthew Wilcox, Jann Horn
  Cc: Dave Hansen, Anthony Liguori, Peter Zijlstra, Catalin Marinas,
	X86 ML, Will Deacon, LKML, Ingo Molnar, Borislav Petkov,
	David Woodhouse, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Thomas Gleixner, linux-arm-kernel

On Wed, Mar 13, 2019 at 3:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi <alisaidi@amazon.com> wrote:
> >
> > Increase mmap_base by the worst-case brk randomization so that
> > the stack and heap remain apart.
> >
> > In Linux 4.13 a change was committed that special cased the kernel ELF
> > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> > ELF_ET_DYN_BASE only for PIE). Generally, the loader isn’t invoked
> > directly and this issue is limited to cases where it is, (e.g to set a
> > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> > those rare cases, the loader doesn't take into account the amount of brk
> > randomization that will be applied by arch_randomize_brk(). This can
> > lead to the stack and heap being arbitrarily close to each other.
>
> In the case of using the loader directly, brk (so helpfully identified
> as "[heap]") is allocated with the _loader_ not the binary. For
> example, with ASLR entirely disabled, you can see this more clearly:
>
> $ /bin/cat /proc/self/maps
> 555555554000-55555555c000 r-xp 00000000 fd:02 34603015
>   /bin/cat
> 55555575b000-55555575c000 r--p 00007000 fd:02 34603015
>   /bin/cat
> 55555575c000-55555575d000 rw-p 00008000 fd:02 34603015
>   /bin/cat
> 55555575d000-55555577e000 rw-p 00000000 00:00 0                          [heap]
> ...
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
> 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> ...
> 7ffff7bcc000-7ffff7bd4000 r-xp 00000000 fd:02 34603015
>   /bin/cat
> 7ffff7bd4000-7ffff7dd3000 ---p 00008000 fd:02 34603015
>   /bin/cat
> 7ffff7dd3000-7ffff7dd4000 r--p 00007000 fd:02 34603015
>   /bin/cat
> 7ffff7dd4000-7ffff7dd5000 rw-p 00008000 fd:02 34603015
>   /bin/cat
> 7ffff7dd5000-7ffff7dfc000 r-xp 00000000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7fb2000-7ffff7fd6000 rw-p 00000000 00:00 0
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff8020000 rw-p 00000000 00:00 0                          [heap]
> 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
>
> So I think changing this globally isn't the right solution (normally
> brk is between text and mmap). Adjusting the mmap base padding means
> we lose even more memory space. Perhaps it would be better if brk
> allocation would be placed before the mmap region (i.e. use
> ELF_ET_DYN_BASE). This seems to work for me:
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 7d09d125f148..cdaa33f4a3ef 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1131,6 +1131,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
>         current->mm->end_data = end_data;
>         current->mm->start_stack = bprm->p;
>
> +       /*
> +        * When executing a loader directly (ET_DYN without Interp), move
> +        * the brk area out of the mmap region (since it grows up, and may
> +        * collide early with the stack growing down), and into the unused
> +        * ELF_ET_DYN_BASE region.
> +        */
> +       if (!elf_interpreter)
> +               current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
> +
>         if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
>                 current->mm->brk = current->mm->start_brk =
>                         arch_randomize_brk(current->mm);
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> 555556de3000-555556e04000 rw-p 00000000 00:00 0                          [heap]
> 7f8467da9000-7f8467f90000 r-xp 00000000 fd:01 399295
>   /lib/x86_64-linux-gnu/libc-2.27.so
> ...
> 7f846819a000-7f84681a2000 r-xp 00000000 fd:01 263229
>   /bin/cat
> ...
> 7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7f84685cc000-7f84685cd000 rw-p 00000000 00:00 0
> 7ffce68f8000-7ffce6919000 rw-p 00000000 00:00 0                          [stack]
> 7ffce69f0000-7ffce69f3000 r--p 00000000 00:00 0                          [vvar]
> 7ffce69f3000-7ffce69f4000 r-xp 00000000 00:00 0                          [vdso]
>
> Does anyone see problems with this? (Note that ET_EXEC base is
> 0x400000, so no collision there...)

Adding some more people to CC... what do people think about this
moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
that worked before should still work (i.e. the ultimately-launched
binary already had the brk very far from its text, so this should be
no different from a COMPAT_BRK standpoint). The only risk I see here
is that if someone started to suddenly depend on the entire memory
space above the mmap region being available when launching binaries
via a direct loader execs... which seems highly unlikely, I'd hope:
this would mean a binary would not work when execed normally.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-03-27 19:51       ` Kees Cook
@ 2019-04-15 16:03         ` Saidi, Ali
  -1 siblings, 0 replies; 26+ messages in thread
From: Saidi, Ali @ 2019-04-15 16:03 UTC (permalink / raw)
  To: Kees Cook, Michal Hocko, Matthew Wilcox, Jann Horn
  Cc: Dave Hansen, Liguori, Anthony, Peter Zijlstra, Catalin Marinas,
	X86 ML, Will Deacon, LKML, Ingo Molnar, Borislav Petkov,
	Woodhouse, David, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Thomas Gleixner, linux-arm-kernel



On 3/27/19, 2:52 PM, "linux-arm-kernel on behalf of Kees Cook" <linux-arm-kernel-bounces@lists.infradead.org on behalf of keescook@chromium.org> wrote:

    Adding some more people to CC... what do people think about this
    moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
    that worked before should still work (i.e. the ultimately-launched
    binary already had the brk very far from its text, so this should be
    no different from a COMPAT_BRK standpoint). The only risk I see here
    is that if someone started to suddenly depend on the entire memory
    space above the mmap region being available when launching binaries
    via a direct loader execs... which seems highly unlikely, I'd hope:
    this would mean a binary would not work when execed normally.

Kees' proposal addresses the issue for me. Anyone have concerns on this proposed solution?

Ali



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-04-15 16:03         ` Saidi, Ali
  0 siblings, 0 replies; 26+ messages in thread
From: Saidi, Ali @ 2019-04-15 16:03 UTC (permalink / raw)
  To: Kees Cook, Michal Hocko, Matthew Wilcox, Jann Horn
  Cc: X86 ML, Andy Lutomirski, Peter Zijlstra, Catalin Marinas,
	Dave Hansen, Will Deacon, LKML, Ingo Molnar, Borislav Petkov,
	linux-arm-kernel, Liguori, Anthony, H. Peter Anvin,
	Andrew Morton, Thomas Gleixner, Woodhouse, David



On 3/27/19, 2:52 PM, "linux-arm-kernel on behalf of Kees Cook" <linux-arm-kernel-bounces@lists.infradead.org on behalf of keescook@chromium.org> wrote:

    Adding some more people to CC... what do people think about this
    moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
    that worked before should still work (i.e. the ultimately-launched
    binary already had the brk very far from its text, so this should be
    no different from a COMPAT_BRK standpoint). The only risk I see here
    is that if someone started to suddenly depend on the entire memory
    space above the mmap region being available when launching binaries
    via a direct loader execs... which seems highly unlikely, I'd hope:
    this would mean a binary would not work when execed normally.

Kees' proposal addresses the issue for me. Anyone have concerns on this proposed solution?

Ali


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-04-15 16:03         ` Saidi, Ali
@ 2019-04-19  8:51           ` Ingo Molnar
  -1 siblings, 0 replies; 26+ messages in thread
From: Ingo Molnar @ 2019-04-19  8:51 UTC (permalink / raw)
  To: Saidi, Ali
  Cc: Kees Cook, Michal Hocko, Matthew Wilcox, Jann Horn, Dave Hansen,
	Liguori, Anthony, Peter Zijlstra, Catalin Marinas, X86 ML,
	Will Deacon, LKML, Ingo Molnar, Borislav Petkov, Woodhouse,
	David, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Thomas Gleixner, linux-arm-kernel


* Saidi, Ali <alisaidi@amazon.com> wrote:

> On 3/27/19, 2:52 PM, "linux-arm-kernel on behalf of Kees Cook" <linux-arm-kernel-bounces@lists.infradead.org on behalf of keescook@chromium.org> wrote:
> 
>     Adding some more people to CC... what do people think about this
>     moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
>     that worked before should still work (i.e. the ultimately-launched
>     binary already had the brk very far from its text, so this should be
>     no different from a COMPAT_BRK standpoint). The only risk I see here
>     is that if someone started to suddenly depend on the entire memory
>     space above the mmap region being available when launching binaries
>     via a direct loader execs... which seems highly unlikely, I'd hope:
>     this would mean a binary would not work when execed normally.
> 
> Kees' proposal addresses the issue for me. Anyone have concerns on this proposed solution?

I'd suggest incorporating all feedback and sending a v2 series - it's 
much easier to get people's attention via code submitted. ;-)

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-04-19  8:51           ` Ingo Molnar
  0 siblings, 0 replies; 26+ messages in thread
From: Ingo Molnar @ 2019-04-19  8:51 UTC (permalink / raw)
  To: Saidi, Ali
  Cc: Michal Hocko, Kees Cook, Jann Horn, Peter Zijlstra,
	Catalin Marinas, Dave Hansen, Will Deacon, X86 ML,
	Matthew Wilcox, LKML, Ingo Molnar, Borislav Petkov,
	linux-arm-kernel, Liguori, Anthony, H. Peter Anvin,
	Andrew Morton, Andy Lutomirski, Thomas Gleixner, Woodhouse,
	David


* Saidi, Ali <alisaidi@amazon.com> wrote:

> On 3/27/19, 2:52 PM, "linux-arm-kernel on behalf of Kees Cook" <linux-arm-kernel-bounces@lists.infradead.org on behalf of keescook@chromium.org> wrote:
> 
>     Adding some more people to CC... what do people think about this
>     moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
>     that worked before should still work (i.e. the ultimately-launched
>     binary already had the brk very far from its text, so this should be
>     no different from a COMPAT_BRK standpoint). The only risk I see here
>     is that if someone started to suddenly depend on the entire memory
>     space above the mmap region being available when launching binaries
>     via a direct loader execs... which seems highly unlikely, I'd hope:
>     this would mean a binary would not work when execed normally.
> 
> Kees' proposal addresses the issue for me. Anyone have concerns on this proposed solution?

I'd suggest incorporating all feedback and sending a v2 series - it's 
much easier to get people's attention via code submitted. ;-)

Thanks,

	Ingo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
  2019-04-19  8:51           ` Ingo Molnar
@ 2019-04-19 15:00             ` Kees Cook
  -1 siblings, 0 replies; 26+ messages in thread
From: Kees Cook @ 2019-04-19 15:00 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Saidi, Ali, Kees Cook, Michal Hocko, Matthew Wilcox, Jann Horn,
	Dave Hansen, Liguori, Anthony, Peter Zijlstra, Catalin Marinas,
	X86 ML, Will Deacon, LKML, Ingo Molnar, Borislav Petkov,
	Woodhouse, David, Andy Lutomirski, H. Peter Anvin, Andrew Morton,
	Thomas Gleixner, linux-arm-kernel

On Fri, Apr 19, 2019 at 3:51 AM Ingo Molnar <mingo@kernel.org> wrote:
> I'd suggest incorporating all feedback and sending a v2 series - it's
> much easier to get people's attention via code submitted. ;-)

I sent my patch out, akpm added it to -mm and then xtensa broke, and
it got removed. Soooo... I'm looking at it again now. I think I might
combine the ideas and in the interpreter case bump the entire mmap
base by the brk randomization size. That should make things less
strange and solve the corner case without reducing available address
space in the general case.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base
@ 2019-04-19 15:00             ` Kees Cook
  0 siblings, 0 replies; 26+ messages in thread
From: Kees Cook @ 2019-04-19 15:00 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-arm-kernel, Michal Hocko, Kees Cook, Jann Horn,
	Peter Zijlstra, Catalin Marinas, Dave Hansen, Will Deacon,
	X86 ML, Matthew Wilcox, LKML, Ingo Molnar, Borislav Petkov,
	Woodhouse, David, Liguori, Anthony, H. Peter Anvin,
	Andrew Morton, Andy Lutomirski, Thomas Gleixner, Saidi, Ali

On Fri, Apr 19, 2019 at 3:51 AM Ingo Molnar <mingo@kernel.org> wrote:
> I'd suggest incorporating all feedback and sending a v2 series - it's
> much easier to get people's attention via code submitted. ;-)

I sent my patch out, akpm added it to -mm and then xtensa broke, and
it got removed. Soooo... I'm looking at it again now. I think I might
combine the ideas and in the interpreter case bump the entire mmap
base by the brk randomization size. That should make things less
strange and solve the corner case without reducing available address
space in the general case.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2019-04-19 19:40 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-12 17:32 [PATCH 0/2] handle worst-case heap randomization in mmap_base Ali Saidi
2019-03-12 17:32 ` Ali Saidi
2019-03-12 17:32 ` [PATCH 1/2] arm64/mmap: " Ali Saidi
2019-03-12 17:32   ` Ali Saidi
2019-03-12 17:32 ` [PATCH 2/2] x86/mmap: " Ali Saidi
2019-03-12 17:32   ` Ali Saidi
2019-03-13 16:25   ` Dave Hansen
2019-03-13 16:25     ` Dave Hansen
2019-03-17 15:52     ` Saidi, Ali
2019-03-17 15:52       ` Saidi, Ali
2019-03-13 22:58   ` Kees Cook
2019-03-13 22:58     ` Kees Cook
2019-03-27 19:51     ` Kees Cook
2019-03-27 19:51       ` Kees Cook
2019-04-15 16:03       ` Saidi, Ali
2019-04-15 16:03         ` Saidi, Ali
2019-04-19  8:51         ` Ingo Molnar
2019-04-19  8:51           ` Ingo Molnar
2019-04-19 15:00           ` Kees Cook
2019-04-19 15:00             ` Kees Cook
2019-03-21 14:09   ` Thomas Gleixner
2019-03-21 14:09     ` Thomas Gleixner
2019-03-26  2:13     ` Saidi, Ali
2019-03-26  2:13       ` Saidi, Ali
2019-03-26  8:43       ` Thomas Gleixner
2019-03-26  8:43         ` Thomas Gleixner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.