2019-04-17 05:24:08

by Alexandre Ghiti

Subject: [PATCH v3 00/11] Provide generic top-down mmap layout functions

This series introduces generic functions to make top-down mmap layout
easily accessible to architectures, in particular riscv which was
the initial goal of this series.
The generic implementation was taken from arm64 and used successively
by arm, mips and finally riscv.

Note that in addition the series fixes 2 issues:
- stack randomization was taken into account even if not necessary.
- [1] fixed an issue with mmap base which did not take into account
randomization but did not report it to arm and mips, so by moving
arm64 into a generic library, this problem is now fixed for both
architectures.

This work is an effort to factorize architecture functions to avoid
code duplication and oversights as in [1].

[1]: https://www.mail-archive.com/[email protected]/msg1429066.html

Changes in v3:
- Split into small patches to ease review as suggested by Christoph
Hellwig and Kees Cook
- Move help text of new config as a comment, as suggested by Christoph
- Make new config depend on MMU, as suggested by Christoph

Changes in v2 as suggested by Christoph Hellwig:
- Preparatory patch that moves randomize_stack_top
- Fix duplicate config in riscv
- Align #if defined on next line => this gives rise to a checkpatch
warning. I found this pattern all around the tree, in the same proportion
as the previous pattern which was less pretty:
git grep -C 1 -n -P "^#if defined.+\|\|.*\\\\$"

Alexandre Ghiti (11):
mm, fs: Move randomize_stack_top from fs to mm
arm64: Make use of is_compat_task instead of hardcoding this test
arm64: Consider stack randomization for mmap base only when necessary
arm64, mm: Move generic mmap layout functions to mm
arm: Properly account for stack randomization and stack guard gap
arm: Use STACK_TOP when computing mmap base address
arm: Use generic mmap top-down layout
mips: Properly account for stack randomization and stack guard gap
mips: Use STACK_TOP when computing mmap base address
mips: Use generic mmap top-down layout
riscv: Make mmap allocation top-down by default

arch/Kconfig | 8 +++
arch/arm/Kconfig | 1 +
arch/arm/include/asm/processor.h | 2 -
arch/arm/mm/mmap.c | 52 ----------------
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/processor.h | 2 -
arch/arm64/mm/mmap.c | 72 ----------------------
arch/mips/Kconfig | 1 +
arch/mips/include/asm/processor.h | 5 --
arch/mips/mm/mmap.c | 57 ------------------
arch/riscv/Kconfig | 11 ++++
fs/binfmt_elf.c | 20 -------
include/linux/mm.h | 2 +
kernel/sysctl.c | 6 +-
mm/util.c | 96 +++++++++++++++++++++++++++++-
15 files changed, 123 insertions(+), 213 deletions(-)

--
2.20.1

2019-04-17 05:25:24

by Alexandre Ghiti

Subject: [PATCH v3 01/11] mm, fs: Move randomize_stack_top from fs to mm

This preparatory commit moves this function so that further introduction
of generic topdown mmap layout is contained only in mm/util.c.

Signed-off-by: Alexandre Ghiti <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/binfmt_elf.c | 20 --------------------
include/linux/mm.h | 2 ++
mm/util.c | 22 ++++++++++++++++++++++
3 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 7d09d125f148..045f3b29d264 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -662,26 +662,6 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
* libraries. There is no binary dependent code anywhere else.
*/

-#ifndef STACK_RND_MASK
-#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12)) /* 8MB of VA */
-#endif
-
-static unsigned long randomize_stack_top(unsigned long stack_top)
-{
- unsigned long random_variable = 0;
-
- if (current->flags & PF_RANDOMIZE) {
- random_variable = get_random_long();
- random_variable &= STACK_RND_MASK;
- random_variable <<= PAGE_SHIFT;
- }
-#ifdef CONFIG_STACK_GROWSUP
- return PAGE_ALIGN(stack_top) + random_variable;
-#else
- return PAGE_ALIGN(stack_top) - random_variable;
-#endif
-}
-
static int load_elf_binary(struct linux_binprm *bprm)
{
struct file *interpreter = NULL; /* to shut gcc up */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 76769749b5a5..087824a5059f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2312,6 +2312,8 @@ extern int install_special_mapping(struct mm_struct *mm,
unsigned long addr, unsigned long len,
unsigned long flags, struct page **pages);

+unsigned long randomize_stack_top(unsigned long stack_top);
+
extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);

extern unsigned long mmap_region(struct file *file, unsigned long addr,
diff --git a/mm/util.c b/mm/util.c
index d559bde497a9..a54afb9b4faa 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -14,6 +14,8 @@
#include <linux/hugetlb.h>
#include <linux/vmalloc.h>
#include <linux/userfaultfd_k.h>
+#include <linux/elf.h>
+#include <linux/random.h>

#include <linux/uaccess.h>

@@ -291,6 +293,26 @@ int vma_is_stack_for_current(struct vm_area_struct *vma)
return (vma->vm_start <= KSTK_ESP(t) && vma->vm_end >= KSTK_ESP(t));
}

+#ifndef STACK_RND_MASK
+#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12)) /* 8MB of VA */
+#endif
+
+unsigned long randomize_stack_top(unsigned long stack_top)
+{
+ unsigned long random_variable = 0;
+
+ if (current->flags & PF_RANDOMIZE) {
+ random_variable = get_random_long();
+ random_variable &= STACK_RND_MASK;
+ random_variable <<= PAGE_SHIFT;
+ }
+#ifdef CONFIG_STACK_GROWSUP
+ return PAGE_ALIGN(stack_top) + random_variable;
+#else
+ return PAGE_ALIGN(stack_top) - random_variable;
+#endif
+}
+
#if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
{
--
2.20.1

2019-04-17 05:27:18

by Alexandre Ghiti

Subject: [PATCH v3 03/11] arm64: Consider stack randomization for mmap base only when necessary

Do not offset mmap base address because of stack randomization if
current task does not want randomization.

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/arm64/mm/mmap.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index ed4f9915f2b8..ac89686c4af8 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -65,7 +65,11 @@ unsigned long arch_mmap_rnd(void)
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;
+ unsigned long pad = stack_guard_gap;
+
+ /* Account for stack randomization if necessary */
+ if (current->flags & PF_RANDOMIZE)
+ pad += (STACK_RND_MASK << PAGE_SHIFT);

/* Values close to RLIM_INFINITY can overflow. */
if (gap + pad > gap)
--
2.20.1

2019-04-17 05:27:31

by Alexandre Ghiti

Subject: [PATCH v3 02/11] arm64: Make use of is_compat_task instead of hardcoding this test

Each architecture has its own way to determine if a task is a compat task,
by using is_compat_task in arch_mmap_rnd, it allows more genericity and
then it prepares its moving to mm/.

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/arm64/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index 842c8a5fcd53..ed4f9915f2b8 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -54,7 +54,7 @@ unsigned long arch_mmap_rnd(void)
unsigned long rnd;

#ifdef CONFIG_COMPAT
- if (test_thread_flag(TIF_32BIT))
+ if (is_compat_task())
rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
else
#endif
--
2.20.1

2019-04-17 05:28:41

by Alexandre Ghiti

Subject: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

arm64 handles top-down mmap layout in a way that can be easily reused
by other architectures, so make it available in mm.
It then introduces a new config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
that can be set by other architectures to benefit from those functions.
Note that this new config depends on MMU being enabled, if selected
without MMU support, a warning will be thrown.

Suggested-by: Christoph Hellwig <[email protected]>
Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/Kconfig | 8 ++++
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/processor.h | 2 -
arch/arm64/mm/mmap.c | 76 ------------------------------
kernel/sysctl.c | 6 ++-
mm/util.c | 74 ++++++++++++++++++++++++++++-
6 files changed, 86 insertions(+), 81 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 33687dddd86a..7c8965c64590 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -684,6 +684,14 @@ config HAVE_ARCH_COMPAT_MMAP_BASES
and vice-versa 32-bit applications to call 64-bit mmap().
Required for applications doing different bitness syscalls.

+# This allows to use a set of generic functions to determine mmap base
+# address by giving priority to top-down scheme only if the process
+# is not in legacy mode (compat task, unlimited stack size or
+# sysctl_legacy_va_layout).
+config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
+ bool
+ depends on MMU
+
config HAVE_COPY_THREAD_TLS
bool
help
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9eba5de..670719a26b45 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -66,6 +66,7 @@ config ARM64
select ARCH_SUPPORTS_INT128 if GCC_VERSION >= 50000 || CC_IS_CLANG
select ARCH_SUPPORTS_NUMA_BALANCING
select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
+ select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
select ARCH_WANT_FRAME_POINTERS
select ARCH_HAS_UBSAN_SANITIZE_ALL
select ARM_AMBA
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 5d9ce62bdebd..4de2a2fd605a 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -274,8 +274,6 @@ static inline void spin_lock_prefetch(const void *ptr)
"nop") : : "p" (ptr));
}

-#define HAVE_ARCH_PICK_MMAP_LAYOUT
-
#endif

extern unsigned long __ro_after_init signal_minsigstksz; /* sigframe size */
diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index ac89686c4af8..c74224421216 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -31,82 +31,6 @@

#include <asm/cputype.h>

-/*
- * Leave enough space between the mmap area and the stack to honour ulimit in
- * the face of randomisation.
- */
-#define MIN_GAP (SZ_128M)
-#define MAX_GAP (STACK_TOP/6*5)
-
-static int mmap_is_legacy(struct rlimit *rlim_stack)
-{
- if (current->personality & ADDR_COMPAT_LAYOUT)
- return 1;
-
- if (rlim_stack->rlim_cur == RLIM_INFINITY)
- return 1;
-
- return sysctl_legacy_va_layout;
-}
-
-unsigned long arch_mmap_rnd(void)
-{
- unsigned long rnd;
-
-#ifdef CONFIG_COMPAT
- if (is_compat_task())
- rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
- else
-#endif
- rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
- return rnd << PAGE_SHIFT;
-}
-
-static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
-{
- unsigned long gap = rlim_stack->rlim_cur;
- unsigned long pad = stack_guard_gap;
-
- /* Account for stack randomization if necessary */
- if (current->flags & PF_RANDOMIZE)
- pad += (STACK_RND_MASK << PAGE_SHIFT);
-
- /* Values close to RLIM_INFINITY can overflow. */
- if (gap + pad > gap)
- gap += pad;
-
- if (gap < MIN_GAP)
- gap = MIN_GAP;
- else if (gap > MAX_GAP)
- gap = MAX_GAP;
-
- return PAGE_ALIGN(STACK_TOP - gap - rnd);
-}
-
-/*
- * This function, called very early during the creation of a new process VM
- * image, sets up which VM layout function to use:
- */
-void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
-{
- unsigned long random_factor = 0UL;
-
- if (current->flags & PF_RANDOMIZE)
- random_factor = arch_mmap_rnd();
-
- /*
- * Fall back to the standard layout if the personality bit is set, or
- * if the expected stack growth is unlimited:
- */
- if (mmap_is_legacy(rlim_stack)) {
- mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
- mm->get_unmapped_area = arch_get_unmapped_area;
- } else {
- mm->mmap_base = mmap_base(random_factor, rlim_stack);
- mm->get_unmapped_area = arch_get_unmapped_area_topdown;
- }
-}
-
/*
* You really shouldn't be using read() or write() on /dev/mem. This might go
* away in the future.
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e5da394d1ca3..eb3414e78986 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -269,7 +269,8 @@ extern struct ctl_table epoll_table[];
extern struct ctl_table firmware_config_table[];
#endif

-#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
+#if defined(HAVE_ARCH_PICK_MMAP_LAYOUT) || \
+ defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
int sysctl_legacy_va_layout;
#endif

@@ -1564,7 +1565,8 @@ static struct ctl_table vm_table[] = {
.proc_handler = proc_dointvec,
.extra1 = &zero,
},
-#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
+#if defined(HAVE_ARCH_PICK_MMAP_LAYOUT) || \
+ defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
{
.procname = "legacy_va_layout",
.data = &sysctl_legacy_va_layout,
diff --git a/mm/util.c b/mm/util.c
index a54afb9b4faa..5c3393d32ed1 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -15,7 +15,12 @@
#include <linux/vmalloc.h>
#include <linux/userfaultfd_k.h>
#include <linux/elf.h>
+#include <linux/elf-randomize.h>
+#include <linux/personality.h>
#include <linux/random.h>
+#include <linux/processor.h>
+#include <linux/sizes.h>
+#include <linux/compat.h>

#include <linux/uaccess.h>

@@ -313,7 +318,74 @@ unsigned long randomize_stack_top(unsigned long stack_top)
#endif
}

-#if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
+#ifdef CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
+#ifdef CONFIG_ARCH_HAS_ELF_RANDOMIZE
+unsigned long arch_mmap_rnd(void)
+{
+ unsigned long rnd;
+
+#ifdef CONFIG_COMPAT
+ if (is_compat_task())
+ rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
+ else
+#endif /* CONFIG_COMPAT */
+ rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
+
+ return rnd << PAGE_SHIFT;
+}
+#endif /* CONFIG_ARCH_HAS_ELF_RANDOMIZE */
+
+static int mmap_is_legacy(struct rlimit *rlim_stack)
+{
+ if (current->personality & ADDR_COMPAT_LAYOUT)
+ return 1;
+
+ if (rlim_stack->rlim_cur == RLIM_INFINITY)
+ return 1;
+
+ return sysctl_legacy_va_layout;
+}
+
+#define MIN_GAP (SZ_128M)
+#define MAX_GAP (STACK_TOP / 6 * 5)
+
+static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
+{
+ unsigned long gap = rlim_stack->rlim_cur;
+ unsigned long pad = stack_guard_gap;
+
+ /* Account for stack randomization if necessary */
+ if (current->flags & PF_RANDOMIZE)
+ pad += (STACK_RND_MASK << PAGE_SHIFT);
+
+ /* Values close to RLIM_INFINITY can overflow. */
+ if (gap + pad > gap)
+ gap += pad;
+
+ if (gap < MIN_GAP)
+ gap = MIN_GAP;
+ else if (gap > MAX_GAP)
+ gap = MAX_GAP;
+
+ return PAGE_ALIGN(STACK_TOP - gap - rnd);
+}
+
+void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
+{
+ unsigned long random_factor = 0UL;
+
+ if (current->flags & PF_RANDOMIZE)
+ random_factor = arch_mmap_rnd();
+
+ if (mmap_is_legacy(rlim_stack)) {
+ mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
+ mm->get_unmapped_area = arch_get_unmapped_area;
+ } else {
+ mm->mmap_base = mmap_base(random_factor, rlim_stack);
+ mm->get_unmapped_area = arch_get_unmapped_area_topdown;
+ }
+}
+#elif defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
{
mm->mmap_base = TASK_UNMAPPED_BASE;
--
2.20.1

2019-04-17 05:29:48

by Alexandre Ghiti

Subject: [PATCH v3 05/11] arm: Properly account for stack randomization and stack guard gap

This commit takes care of stack randomization and stack guard gap when
computing mmap base address and checks if the task asked for randomization.
This fixes the problem uncovered and not fixed for arm here:
https://www.mail-archive.com/[email protected]/msg1429066.html

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/arm/mm/mmap.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index f866870db749..bff3d00bda5b 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -18,8 +18,9 @@
(((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))

/* gap between mmap and stack */
-#define MIN_GAP (128*1024*1024UL)
-#define MAX_GAP ((TASK_SIZE)/6*5)
+#define MIN_GAP (128*1024*1024UL)
+#define MAX_GAP ((TASK_SIZE)/6*5)
+#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))

static int mmap_is_legacy(struct rlimit *rlim_stack)
{
@@ -35,6 +36,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
{
unsigned long gap = rlim_stack->rlim_cur;
+ unsigned long pad = stack_guard_gap;
+
+ /* Account for stack randomization if necessary */
+ if (current->flags & PF_RANDOMIZE)
+ pad += (STACK_RND_MASK << PAGE_SHIFT);
+
+ /* Values close to RLIM_INFINITY can overflow. */
+ if (gap + pad > gap)
+ gap += pad;

if (gap < MIN_GAP)
gap = MIN_GAP;
--
2.20.1

2019-04-17 05:31:00

by Alexandre Ghiti

Subject: [PATCH v3 06/11] arm: Use STACK_TOP when computing mmap base address

mmap base address must be computed wrt stack top address, using TASK_SIZE
is wrong since STACK_TOP and TASK_SIZE are not equivalent.

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/arm/mm/mmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index bff3d00bda5b..0b94b674aa91 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -19,7 +19,7 @@

/* gap between mmap and stack */
#define MIN_GAP (128*1024*1024UL)
-#define MAX_GAP ((TASK_SIZE)/6*5)
+#define MAX_GAP ((STACK_TOP)/6*5)
#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))

static int mmap_is_legacy(struct rlimit *rlim_stack)
@@ -51,7 +51,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
else if (gap > MAX_GAP)
gap = MAX_GAP;

- return PAGE_ALIGN(TASK_SIZE - gap - rnd);
+ return PAGE_ALIGN(STACK_TOP - gap - rnd);
}

/*
--
2.20.1

2019-04-17 05:32:05

by Alexandre Ghiti

Subject: [PATCH v3 07/11] arm: Use generic mmap top-down layout

arm uses a top-down mmap layout by default that exactly fits the generic
functions, so get rid of arch specific code and use the generic version
by selecting ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT.

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/arm/Kconfig | 1 +
arch/arm/include/asm/processor.h | 2 --
arch/arm/mm/mmap.c | 62 --------------------------------
3 files changed, 1 insertion(+), 64 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 850b4805e2d1..f8f603da181f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -28,6 +28,7 @@ config ARM
select ARCH_SUPPORTS_ATOMIC_RMW
select ARCH_USE_BUILTIN_BSWAP
select ARCH_USE_CMPXCHG_LOCKREF
+ select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
select ARCH_WANT_IPC_PARSE_VERSION
select BUILDTIME_EXTABLE_SORT if MMU
select CLONE_BACKWARDS
diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 57fe73ea0f72..944ef1fb1237 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -143,8 +143,6 @@ static inline void prefetchw(const void *ptr)
#endif
#endif

-#define HAVE_ARCH_PICK_MMAP_LAYOUT
-
#endif

#endif /* __ASM_ARM_PROCESSOR_H */
diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index 0b94b674aa91..b8d912ac9e61 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -17,43 +17,6 @@
((((addr)+SHMLBA-1)&~(SHMLBA-1)) + \
(((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))

-/* gap between mmap and stack */
-#define MIN_GAP (128*1024*1024UL)
-#define MAX_GAP ((STACK_TOP)/6*5)
-#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
-
-static int mmap_is_legacy(struct rlimit *rlim_stack)
-{
- if (current->personality & ADDR_COMPAT_LAYOUT)
- return 1;
-
- if (rlim_stack->rlim_cur == RLIM_INFINITY)
- return 1;
-
- return sysctl_legacy_va_layout;
-}
-
-static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
-{
- unsigned long gap = rlim_stack->rlim_cur;
- unsigned long pad = stack_guard_gap;
-
- /* Account for stack randomization if necessary */
- if (current->flags & PF_RANDOMIZE)
- pad += (STACK_RND_MASK << PAGE_SHIFT);
-
- /* Values close to RLIM_INFINITY can overflow. */
- if (gap + pad > gap)
- gap += pad;
-
- if (gap < MIN_GAP)
- gap = MIN_GAP;
- else if (gap > MAX_GAP)
- gap = MAX_GAP;
-
- return PAGE_ALIGN(STACK_TOP - gap - rnd);
-}
-
/*
* We need to ensure that shared mappings are correctly aligned to
* avoid aliasing issues with VIPT caches. We need to ensure that
@@ -181,31 +144,6 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
return addr;
}

-unsigned long arch_mmap_rnd(void)
-{
- unsigned long rnd;
-
- rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
-
- return rnd << PAGE_SHIFT;
-}
-
-void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
-{
- unsigned long random_factor = 0UL;
-
- if (current->flags & PF_RANDOMIZE)
- random_factor = arch_mmap_rnd();
-
- if (mmap_is_legacy(rlim_stack)) {
- mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
- mm->get_unmapped_area = arch_get_unmapped_area;
- } else {
- mm->mmap_base = mmap_base(random_factor, rlim_stack);
- mm->get_unmapped_area = arch_get_unmapped_area_topdown;
- }
-}
-
/*
* You really shouldn't be using read() or write() on /dev/mem. This
* might go away in the future.
--
2.20.1

2019-04-17 05:33:11

by Alexandre Ghiti

Subject: [PATCH v3 08/11] mips: Properly account for stack randomization and stack guard gap

This commit takes care of stack randomization and stack guard gap when
computing mmap base address and checks if the task asked for randomization.
This fixes the problem uncovered and not fixed for mips here:
https://www.mail-archive.com/[email protected]/msg1429066.html

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/mips/mm/mmap.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index 2f616ebeb7e0..3ff82c6f7e24 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -21,8 +21,9 @@ unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
EXPORT_SYMBOL(shm_align_mask);

/* gap between mmap and stack */
-#define MIN_GAP (128*1024*1024UL)
-#define MAX_GAP ((TASK_SIZE)/6*5)
+#define MIN_GAP (128*1024*1024UL)
+#define MAX_GAP ((TASK_SIZE)/6*5)
+#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))

static int mmap_is_legacy(struct rlimit *rlim_stack)
{
@@ -38,6 +39,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
{
unsigned long gap = rlim_stack->rlim_cur;
+ unsigned long pad = stack_guard_gap;
+
+ /* Account for stack randomization if necessary */
+ if (current->flags & PF_RANDOMIZE)
+ pad += (STACK_RND_MASK << PAGE_SHIFT);
+
+ /* Values close to RLIM_INFINITY can overflow. */
+ if (gap + pad > gap)
+ gap += pad;

if (gap < MIN_GAP)
gap = MIN_GAP;
--
2.20.1

2019-04-17 05:34:29

by Alexandre Ghiti

Subject: [PATCH v3 09/11] mips: Use STACK_TOP when computing mmap base address

mmap base address must be computed wrt stack top address, using TASK_SIZE
is wrong since STACK_TOP and TASK_SIZE are not equivalent.

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/mips/mm/mmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index 3ff82c6f7e24..ffbe69f3a7d9 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -22,7 +22,7 @@ EXPORT_SYMBOL(shm_align_mask);

/* gap between mmap and stack */
#define MIN_GAP (128*1024*1024UL)
-#define MAX_GAP ((TASK_SIZE)/6*5)
+#define MAX_GAP ((STACK_TOP)/6*5)
#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))

static int mmap_is_legacy(struct rlimit *rlim_stack)
@@ -54,7 +54,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
else if (gap > MAX_GAP)
gap = MAX_GAP;

- return PAGE_ALIGN(TASK_SIZE - gap - rnd);
+ return PAGE_ALIGN(STACK_TOP - gap - rnd);
}

#define COLOUR_ALIGN(addr, pgoff) \
--
2.20.1

2019-04-17 05:35:36

by Alexandre Ghiti

Subject: [PATCH v3 10/11] mips: Use generic mmap top-down layout

mips uses a top-down layout by default that fits the generic functions.
At the same time, this commit allows to fix problem uncovered
and not fixed for mips here:
https://www.mail-archive.com/[email protected]/msg1429066.html

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/mips/Kconfig | 1 +
arch/mips/include/asm/processor.h | 5 ---
arch/mips/mm/mmap.c | 67 -------------------------------
3 files changed, 1 insertion(+), 72 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 4a5f5b0ee9a9..ec2f07561e4d 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -14,6 +14,7 @@ config MIPS
select ARCH_USE_CMPXCHG_LOCKREF if 64BIT
select ARCH_USE_QUEUED_RWLOCKS
select ARCH_USE_QUEUED_SPINLOCKS
+ select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
select ARCH_WANT_IPC_PARSE_VERSION
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/arch/mips/include/asm/processor.h b/arch/mips/include/asm/processor.h
index aca909bd7841..fba18d4a9190 100644
--- a/arch/mips/include/asm/processor.h
+++ b/arch/mips/include/asm/processor.h
@@ -29,11 +29,6 @@

extern unsigned int vced_count, vcei_count;

-/*
- * MIPS does have an arch_pick_mmap_layout()
- */
-#define HAVE_ARCH_PICK_MMAP_LAYOUT 1
-
#ifdef CONFIG_32BIT
#ifdef CONFIG_KVM_GUEST
/* User space process size is limited to 1GB in KVM Guest Mode */
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index ffbe69f3a7d9..61e65a69bb09 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -20,43 +20,6 @@
unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
EXPORT_SYMBOL(shm_align_mask);

-/* gap between mmap and stack */
-#define MIN_GAP (128*1024*1024UL)
-#define MAX_GAP ((STACK_TOP)/6*5)
-#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
-
-static int mmap_is_legacy(struct rlimit *rlim_stack)
-{
- if (current->personality & ADDR_COMPAT_LAYOUT)
- return 1;
-
- if (rlim_stack->rlim_cur == RLIM_INFINITY)
- return 1;
-
- return sysctl_legacy_va_layout;
-}
-
-static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
-{
- unsigned long gap = rlim_stack->rlim_cur;
- unsigned long pad = stack_guard_gap;
-
- /* Account for stack randomization if necessary */
- if (current->flags & PF_RANDOMIZE)
- pad += (STACK_RND_MASK << PAGE_SHIFT);
-
- /* Values close to RLIM_INFINITY can overflow. */
- if (gap + pad > gap)
- gap += pad;
-
- if (gap < MIN_GAP)
- gap = MIN_GAP;
- else if (gap > MAX_GAP)
- gap = MAX_GAP;
-
- return PAGE_ALIGN(STACK_TOP - gap - rnd);
-}
-
#define COLOUR_ALIGN(addr, pgoff) \
((((addr) + shm_align_mask) & ~shm_align_mask) + \
(((pgoff) << PAGE_SHIFT) & shm_align_mask))
@@ -154,36 +117,6 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp,
addr0, len, pgoff, flags, DOWN);
}

-unsigned long arch_mmap_rnd(void)
-{
- unsigned long rnd;
-
-#ifdef CONFIG_COMPAT
- if (TASK_IS_32BIT_ADDR)
- rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
- else
-#endif /* CONFIG_COMPAT */
- rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
-
- return rnd << PAGE_SHIFT;
-}
-
-void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
-{
- unsigned long random_factor = 0UL;
-
- if (current->flags & PF_RANDOMIZE)
- random_factor = arch_mmap_rnd();
-
- if (mmap_is_legacy(rlim_stack)) {
- mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
- mm->get_unmapped_area = arch_get_unmapped_area;
- } else {
- mm->mmap_base = mmap_base(random_factor, rlim_stack);
- mm->get_unmapped_area = arch_get_unmapped_area_topdown;
- }
-}
-
static inline unsigned long brk_rnd(void)
{
unsigned long rnd = get_random_long();
--
2.20.1

2019-04-17 05:36:49

by Alexandre Ghiti

Subject: [PATCH v3 11/11] riscv: Make mmap allocation top-down by default

In order to avoid wasting user address space by using bottom-up mmap
allocation scheme, prefer top-down scheme when possible.

Before:
root@qemuriscv64:~# cat /proc/self/maps
00010000-00016000 r-xp 00000000 fe:00 6389 /bin/cat.coreutils
00016000-00017000 r--p 00005000 fe:00 6389 /bin/cat.coreutils
00017000-00018000 rw-p 00006000 fe:00 6389 /bin/cat.coreutils
00018000-00039000 rw-p 00000000 00:00 0 [heap]
1555556000-155556d000 r-xp 00000000 fe:00 7193 /lib/ld-2.28.so
155556d000-155556e000 r--p 00016000 fe:00 7193 /lib/ld-2.28.so
155556e000-155556f000 rw-p 00017000 fe:00 7193 /lib/ld-2.28.so
155556f000-1555570000 rw-p 00000000 00:00 0
1555570000-1555572000 r-xp 00000000 00:00 0 [vdso]
1555574000-1555576000 rw-p 00000000 00:00 0
1555576000-1555674000 r-xp 00000000 fe:00 7187 /lib/libc-2.28.so
1555674000-1555678000 r--p 000fd000 fe:00 7187 /lib/libc-2.28.so
1555678000-155567a000 rw-p 00101000 fe:00 7187 /lib/libc-2.28.so
155567a000-15556a0000 rw-p 00000000 00:00 0
3fffb90000-3fffbb1000 rw-p 00000000 00:00 0 [stack]

After:
root@qemuriscv64:~# cat /proc/self/maps
00010000-00016000 r-xp 00000000 fe:00 6389 /bin/cat.coreutils
00016000-00017000 r--p 00005000 fe:00 6389 /bin/cat.coreutils
00017000-00018000 rw-p 00006000 fe:00 6389 /bin/cat.coreutils
00018000-00039000 rw-p 00000000 00:00 0 [heap]
3ff7eb6000-3ff7ed8000 rw-p 00000000 00:00 0
3ff7ed8000-3ff7fd6000 r-xp 00000000 fe:00 7187 /lib/libc-2.28.so
3ff7fd6000-3ff7fda000 r--p 000fd000 fe:00 7187 /lib/libc-2.28.so
3ff7fda000-3ff7fdc000 rw-p 00101000 fe:00 7187 /lib/libc-2.28.so
3ff7fdc000-3ff7fe2000 rw-p 00000000 00:00 0
3ff7fe4000-3ff7fe6000 r-xp 00000000 00:00 0 [vdso]
3ff7fe6000-3ff7ffd000 r-xp 00000000 fe:00 7193 /lib/ld-2.28.so
3ff7ffd000-3ff7ffe000 r--p 00016000 fe:00 7193 /lib/ld-2.28.so
3ff7ffe000-3ff7fff000 rw-p 00017000 fe:00 7193 /lib/ld-2.28.so
3ff7fff000-3ff8000000 rw-p 00000000 00:00 0
3fff888000-3fff8a9000 rw-p 00000000 00:00 0 [stack]

Signed-off-by: Alexandre Ghiti <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
arch/riscv/Kconfig | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index eb56c82d8aa1..f5897e0dbc1c 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -49,6 +49,17 @@ config RISCV
select GENERIC_IRQ_MULTI_HANDLER
select ARCH_HAS_PTE_SPECIAL
select HAVE_EBPF_JIT if 64BIT
+ select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
+ select HAVE_ARCH_MMAP_RND_BITS
+
+config ARCH_MMAP_RND_BITS_MIN
+ default 18
+
+# max bits determined by the following formula:
+# VA_BITS - PAGE_SHIFT - 3
+config ARCH_MMAP_RND_BITS_MAX
+ default 33 if 64BIT # SV48 based
+ default 18

config MMU
def_bool y
--
2.20.1

2019-04-18 04:33:39

by Kees Cook

Subject: Re: [PATCH v3 02/11] arm64: Make use of is_compat_task instead of hardcoding this test

On Wed, Apr 17, 2019 at 12:25 AM Alexandre Ghiti <[email protected]> wrote:
>
> Each architecture has its own way to determine if a task is a compat task,
> by using is_compat_task in arch_mmap_rnd, it allows more genericity and
> then it prepares its moving to mm/.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/arm64/mm/mmap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index 842c8a5fcd53..ed4f9915f2b8 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -54,7 +54,7 @@ unsigned long arch_mmap_rnd(void)
> unsigned long rnd;
>
> #ifdef CONFIG_COMPAT
> - if (test_thread_flag(TIF_32BIT))
> + if (is_compat_task())
> rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> else
> #endif
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 04:40:30

by Kees Cook

Subject: Re: [PATCH v3 03/11] arm64: Consider stack randomization for mmap base only when necessary

On Wed, Apr 17, 2019 at 12:26 AM Alexandre Ghiti <[email protected]> wrote:
>
> Do not offset mmap base address because of stack randomization if
> current task does not want randomization.

Maybe mention that this makes this logic match the existing x86 behavior too?

> Signed-off-by: Alexandre Ghiti <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/arm64/mm/mmap.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index ed4f9915f2b8..ac89686c4af8 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -65,7 +65,11 @@ unsigned long arch_mmap_rnd(void)
> 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;
> + unsigned long pad = stack_guard_gap;
> +
> + /* Account for stack randomization if necessary */
> + if (current->flags & PF_RANDOMIZE)
> + pad += (STACK_RND_MASK << PAGE_SHIFT);
>
> /* Values close to RLIM_INFINITY can overflow. */
> if (gap + pad > gap)
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:22:14

by Kees Cook

Subject: Re: [PATCH v3 01/11] mm, fs: Move randomize_stack_top from fs to mm

On Wed, Apr 17, 2019 at 12:24 AM Alexandre Ghiti <[email protected]> wrote:
>
> This preparatory commit moves this function so that further introduction
> of generic topdown mmap layout is contained only in mm/util.c.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> ---
> fs/binfmt_elf.c | 20 --------------------
> include/linux/mm.h | 2 ++
> mm/util.c | 22 ++++++++++++++++++++++
> 3 files changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 7d09d125f148..045f3b29d264 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -662,26 +662,6 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> * libraries. There is no binary dependent code anywhere else.
> */
>
> -#ifndef STACK_RND_MASK
> -#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12)) /* 8MB of VA */
> -#endif
> -
> -static unsigned long randomize_stack_top(unsigned long stack_top)
> -{
> - unsigned long random_variable = 0;
> -
> - if (current->flags & PF_RANDOMIZE) {
> - random_variable = get_random_long();
> - random_variable &= STACK_RND_MASK;
> - random_variable <<= PAGE_SHIFT;
> - }
> -#ifdef CONFIG_STACK_GROWSUP
> - return PAGE_ALIGN(stack_top) + random_variable;
> -#else
> - return PAGE_ALIGN(stack_top) - random_variable;
> -#endif
> -}
> -
> static int load_elf_binary(struct linux_binprm *bprm)
> {
> struct file *interpreter = NULL; /* to shut gcc up */
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 76769749b5a5..087824a5059f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2312,6 +2312,8 @@ extern int install_special_mapping(struct mm_struct *mm,
> unsigned long addr, unsigned long len,
> unsigned long flags, struct page **pages);
>
> +unsigned long randomize_stack_top(unsigned long stack_top);
> +
> extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
>
> extern unsigned long mmap_region(struct file *file, unsigned long addr,
> diff --git a/mm/util.c b/mm/util.c
> index d559bde497a9..a54afb9b4faa 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -14,6 +14,8 @@
> #include <linux/hugetlb.h>
> #include <linux/vmalloc.h>
> #include <linux/userfaultfd_k.h>
> +#include <linux/elf.h>
> +#include <linux/random.h>
>
> #include <linux/uaccess.h>
>
> @@ -291,6 +293,26 @@ int vma_is_stack_for_current(struct vm_area_struct *vma)
> return (vma->vm_start <= KSTK_ESP(t) && vma->vm_end >= KSTK_ESP(t));
> }
>
> +#ifndef STACK_RND_MASK
> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12)) /* 8MB of VA */
> +#endif

Oh right, here's the generic one... this should probably just copy
arm64's version instead. Then x86 can be tweaked (it uses
mmap_is_ia32() instead of is_compat_task() by default, but has a weird
override..)

Regardless, yes, this is a direct code move:

Acked-by: Kees Cook <[email protected]>

-Kees

> +
> +unsigned long randomize_stack_top(unsigned long stack_top)
> +{
> + unsigned long random_variable = 0;
> +
> + if (current->flags & PF_RANDOMIZE) {
> + random_variable = get_random_long();
> + random_variable &= STACK_RND_MASK;
> + random_variable <<= PAGE_SHIFT;
> + }
> +#ifdef CONFIG_STACK_GROWSUP
> + return PAGE_ALIGN(stack_top) + random_variable;
> +#else
> + return PAGE_ALIGN(stack_top) - random_variable;
> +#endif
> +}
> +
> #if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
> void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
> {
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:24:52

by Alexandre Ghiti

Subject: Re: [PATCH v3 02/11] arm64: Make use of is_compat_task instead of hardcoding this test

On 4/18/19 12:32 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:25 AM Alexandre Ghiti <[email protected]> wrote:
>> Each architecture has its own way to determine if a task is a compat task,
>> by using is_compat_task in arch_mmap_rnd, it allows more genericity and
>> then it prepares its moving to mm/.
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>


Thanks !


> -Kees
>
>> ---
>> arch/arm64/mm/mmap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
>> index 842c8a5fcd53..ed4f9915f2b8 100644
>> --- a/arch/arm64/mm/mmap.c
>> +++ b/arch/arm64/mm/mmap.c
>> @@ -54,7 +54,7 @@ unsigned long arch_mmap_rnd(void)
>> unsigned long rnd;
>>
>> #ifdef CONFIG_COMPAT
>> - if (test_thread_flag(TIF_32BIT))
>> + if (is_compat_task())
>> rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
>> else
>> #endif
>> --
>> 2.20.1
>>
>

2019-04-18 05:26:57

by Kees Cook

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

(

On Wed, Apr 17, 2019 at 12:27 AM Alexandre Ghiti <[email protected]> wrote:
>
> arm64 handles top-down mmap layout in a way that can be easily reused
> by other architectures, so make it available in mm.
> It then introduces a new config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> that can be set by other architectures to benefit from those functions.
> Note that this new config depends on MMU being enabled, if selected
> without MMU support, a warning will be thrown.
>
> Suggested-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> ---
> arch/Kconfig | 8 ++++
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/processor.h | 2 -
> arch/arm64/mm/mmap.c | 76 ------------------------------
> kernel/sysctl.c | 6 ++-
> mm/util.c | 74 ++++++++++++++++++++++++++++-
> 6 files changed, 86 insertions(+), 81 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 33687dddd86a..7c8965c64590 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -684,6 +684,14 @@ config HAVE_ARCH_COMPAT_MMAP_BASES
> and vice-versa 32-bit applications to call 64-bit mmap().
> Required for applications doing different bitness syscalls.
>
> +# This allows to use a set of generic functions to determine mmap base
> +# address by giving priority to top-down scheme only if the process
> +# is not in legacy mode (compat task, unlimited stack size or
> +# sysctl_legacy_va_layout).
> +config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> + bool
> + depends on MMU

I'd prefer the comment were moved to the help text. I would include
any details about what the arch still needs to define. For example
right now, I think STACK_RND_MASK is still needed. (Though I think a
common one could be added for this series too...)

> +
> config HAVE_COPY_THREAD_TLS
> bool
> help
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7e34b9eba5de..670719a26b45 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -66,6 +66,7 @@ config ARM64
> select ARCH_SUPPORTS_INT128 if GCC_VERSION >= 50000 || CC_IS_CLANG
> select ARCH_SUPPORTS_NUMA_BALANCING
> select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> select ARCH_WANT_FRAME_POINTERS
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> select ARM_AMBA
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 5d9ce62bdebd..4de2a2fd605a 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -274,8 +274,6 @@ static inline void spin_lock_prefetch(const void *ptr)
> "nop") : : "p" (ptr));
> }
>
> -#define HAVE_ARCH_PICK_MMAP_LAYOUT
> -
> #endif
>
> extern unsigned long __ro_after_init signal_minsigstksz; /* sigframe size */
> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index ac89686c4af8..c74224421216 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -31,82 +31,6 @@
>
> #include <asm/cputype.h>
>
> -/*
> - * Leave enough space between the mmap area and the stack to honour ulimit in
> - * the face of randomisation.
> - */

This comment goes missing in the move...

> -#define MIN_GAP (SZ_128M)
> -#define MAX_GAP (STACK_TOP/6*5)
> -
> -static int mmap_is_legacy(struct rlimit *rlim_stack)
> -{
> - if (current->personality & ADDR_COMPAT_LAYOUT)
> - return 1;
> -
> - if (rlim_stack->rlim_cur == RLIM_INFINITY)
> - return 1;
> -
> - return sysctl_legacy_va_layout;
> -}
> -
> -unsigned long arch_mmap_rnd(void)
> -{
> - unsigned long rnd;
> -
> -#ifdef CONFIG_COMPAT
> - if (is_compat_task())
> - rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> - else
> -#endif
> - rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> - return rnd << PAGE_SHIFT;
> -}
> -
> -static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> -{
> - unsigned long gap = rlim_stack->rlim_cur;
> - unsigned long pad = stack_guard_gap;
> -
> - /* Account for stack randomization if necessary */
> - if (current->flags & PF_RANDOMIZE)
> - pad += (STACK_RND_MASK << PAGE_SHIFT);
> -
> - /* Values close to RLIM_INFINITY can overflow. */
> - if (gap + pad > gap)
> - gap += pad;
> -
> - if (gap < MIN_GAP)
> - gap = MIN_GAP;
> - else if (gap > MAX_GAP)
> - gap = MAX_GAP;
> -
> - return PAGE_ALIGN(STACK_TOP - gap - rnd);
> -}
> -
> -/*
> - * This function, called very early during the creation of a new process VM
> - * image, sets up which VM layout function to use:
> - */
> -void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
> -{
> - unsigned long random_factor = 0UL;
> -
> - if (current->flags & PF_RANDOMIZE)
> - random_factor = arch_mmap_rnd();
> -
> - /*
> - * Fall back to the standard layout if the personality bit is set, or
> - * if the expected stack growth is unlimited:
> - */
> - if (mmap_is_legacy(rlim_stack)) {
> - mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
> - mm->get_unmapped_area = arch_get_unmapped_area;
> - } else {
> - mm->mmap_base = mmap_base(random_factor, rlim_stack);
> - mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> - }
> -}
> -
> /*
> * You really shouldn't be using read() or write() on /dev/mem. This might go
> * away in the future.
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e5da394d1ca3..eb3414e78986 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -269,7 +269,8 @@ extern struct ctl_table epoll_table[];
> extern struct ctl_table firmware_config_table[];
> #endif
>
> -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
> +#if defined(HAVE_ARCH_PICK_MMAP_LAYOUT) || \
> + defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
> int sysctl_legacy_va_layout;
> #endif
>
> @@ -1564,7 +1565,8 @@ static struct ctl_table vm_table[] = {
> .proc_handler = proc_dointvec,
> .extra1 = &zero,
> },
> -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
> +#if defined(HAVE_ARCH_PICK_MMAP_LAYOUT) || \
> + defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
> {
> .procname = "legacy_va_layout",
> .data = &sysctl_legacy_va_layout,
> diff --git a/mm/util.c b/mm/util.c
> index a54afb9b4faa..5c3393d32ed1 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -15,7 +15,12 @@
> #include <linux/vmalloc.h>
> #include <linux/userfaultfd_k.h>
> #include <linux/elf.h>
> +#include <linux/elf-randomize.h>
> +#include <linux/personality.h>
> #include <linux/random.h>
> +#include <linux/processor.h>
> +#include <linux/sizes.h>
> +#include <linux/compat.h>
>
> #include <linux/uaccess.h>
>
> @@ -313,7 +318,74 @@ unsigned long randomize_stack_top(unsigned long stack_top)
> #endif
> }
>
> -#if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
> +#ifdef CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> +#ifdef CONFIG_ARCH_HAS_ELF_RANDOMIZE

I think CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT should select
CONFIG_ARCH_HAS_ELF_RANDOMIZE. It would mean moving
arch_randomize_brk() into this patch set too. For arm64 and arm, this
is totally fine: they have identical logic. On MIPS this would mean
bumping the randomization up: arm64 uses SZ_32M for 32-bit and SZ_1G
for 64-bit. MIPS is 8M and 256M respectively. I don't see anything
that indicates this would be a problem. *cross fingers*

It looks like x86 would need bumping too: it uses 32M on both 32-bit
and 64-bit. STACK_RND_MASK is the same though.

> +unsigned long arch_mmap_rnd(void)
> +{
> + unsigned long rnd;
> +
> +#ifdef CONFIG_COMPAT
> + if (is_compat_task())
> + rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> + else
> +#endif /* CONFIG_COMPAT */

The ifdefs on is_compat_task() are not needed: is_compat_task()
returns 0 in the !CONFIG_COMPAT case.

> + rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> +
> + return rnd << PAGE_SHIFT;
> +}
> +#endif /* CONFIG_ARCH_HAS_ELF_RANDOMIZE */
> +
> +static int mmap_is_legacy(struct rlimit *rlim_stack)
> +{
> + if (current->personality & ADDR_COMPAT_LAYOUT)
> + return 1;
> +
> + if (rlim_stack->rlim_cur == RLIM_INFINITY)
> + return 1;
> +
> + return sysctl_legacy_va_layout;
> +}
> +
> +#define MIN_GAP (SZ_128M)
> +#define MAX_GAP (STACK_TOP / 6 * 5)
> +
> +static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> +{
> + unsigned long gap = rlim_stack->rlim_cur;
> + unsigned long pad = stack_guard_gap;
> +
> + /* Account for stack randomization if necessary */
> + if (current->flags & PF_RANDOMIZE)
> + pad += (STACK_RND_MASK << PAGE_SHIFT);
> +
> + /* Values close to RLIM_INFINITY can overflow. */
> + if (gap + pad > gap)
> + gap += pad;
> +
> + if (gap < MIN_GAP)
> + gap = MIN_GAP;
> + else if (gap > MAX_GAP)
> + gap = MAX_GAP;
> +
> + return PAGE_ALIGN(STACK_TOP - gap - rnd);
> +}
> +
> +void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
> +{
> + unsigned long random_factor = 0UL;
> +
> + if (current->flags & PF_RANDOMIZE)
> + random_factor = arch_mmap_rnd();
> +
> + if (mmap_is_legacy(rlim_stack)) {
> + mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
> + mm->get_unmapped_area = arch_get_unmapped_area;
> + } else {
> + mm->mmap_base = mmap_base(random_factor, rlim_stack);
> + mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> + }
> +}
> +#elif defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
> void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
> {
> mm->mmap_base = TASK_UNMAPPED_BASE;
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:27:13

by Alexandre Ghiti

Subject: Re: [PATCH v3 03/11] arm64: Consider stack randomization for mmap base only when necessary

On 4/18/19 12:37 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:26 AM Alexandre Ghiti <[email protected]> wrote:
>> Do not offset mmap base address because of stack randomization if
>> current task does not want randomization.
> Maybe mention that this makes this logic match the existing x86 behavior too?


Ok I will add this in case of a v4.


>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>

Thanks !


>
> -Kees
>
>> ---
>> arch/arm64/mm/mmap.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
>> index ed4f9915f2b8..ac89686c4af8 100644
>> --- a/arch/arm64/mm/mmap.c
>> +++ b/arch/arm64/mm/mmap.c
>> @@ -65,7 +65,11 @@ unsigned long arch_mmap_rnd(void)
>> 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;
>> + unsigned long pad = stack_guard_gap;
>> +
>> + /* Account for stack randomization if necessary */
>> + if (current->flags & PF_RANDOMIZE)
>> + pad += (STACK_RND_MASK << PAGE_SHIFT);
>>
>> /* Values close to RLIM_INFINITY can overflow. */
>> if (gap + pad > gap)
>> --
>> 2.20.1
>>
>

2019-04-18 05:29:10

by Kees Cook

Subject: Re: [PATCH v3 06/11] arm: Use STACK_TOP when computing mmap base address

On Wed, Apr 17, 2019 at 12:29 AM Alexandre Ghiti <[email protected]> wrote:
>
> mmap base address must be computed wrt stack top address, using TASK_SIZE
> is wrong since STACK_TOP and TASK_SIZE are not equivalent.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> ---
> arch/arm/mm/mmap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
> index bff3d00bda5b..0b94b674aa91 100644
> --- a/arch/arm/mm/mmap.c
> +++ b/arch/arm/mm/mmap.c
> @@ -19,7 +19,7 @@
>
> /* gap between mmap and stack */
> #define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define MAX_GAP ((STACK_TOP)/6*5)

Parens around STACK_TOP aren't needed, but you'll be removing it
entirely, so I can't complain. ;)

> #define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>
> static int mmap_is_legacy(struct rlimit *rlim_stack)
> @@ -51,7 +51,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> else if (gap > MAX_GAP)
> gap = MAX_GAP;
>
> - return PAGE_ALIGN(TASK_SIZE - gap - rnd);
> + return PAGE_ALIGN(STACK_TOP - gap - rnd);
> }
>
> /*
> --
> 2.20.1
>

Acked-by: Kees Cook <[email protected]>

--
Kees Cook

2019-04-18 05:33:37

by Kees Cook

Subject: Re: [PATCH v3 10/11] mips: Use generic mmap top-down layout

On Wed, Apr 17, 2019 at 12:33 AM Alexandre Ghiti <[email protected]> wrote:
>
> mips uses a top-down layout by default that fits the generic functions.
> At the same time, this commit allows to fix problem uncovered
> and not fixed for mips here:
> https://www.mail-archive.com/[email protected]/msg1429066.html
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/mips/Kconfig | 1 +
> arch/mips/include/asm/processor.h | 5 ---
> arch/mips/mm/mmap.c | 67 -------------------------------
> 3 files changed, 1 insertion(+), 72 deletions(-)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 4a5f5b0ee9a9..ec2f07561e4d 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -14,6 +14,7 @@ config MIPS
> select ARCH_USE_CMPXCHG_LOCKREF if 64BIT
> select ARCH_USE_QUEUED_RWLOCKS
> select ARCH_USE_QUEUED_SPINLOCKS
> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
> select ARCH_WANT_IPC_PARSE_VERSION
> select BUILDTIME_EXTABLE_SORT
> select CLONE_BACKWARDS
> diff --git a/arch/mips/include/asm/processor.h b/arch/mips/include/asm/processor.h
> index aca909bd7841..fba18d4a9190 100644
> --- a/arch/mips/include/asm/processor.h
> +++ b/arch/mips/include/asm/processor.h
> @@ -29,11 +29,6 @@
>
> extern unsigned int vced_count, vcei_count;
>
> -/*
> - * MIPS does have an arch_pick_mmap_layout()
> - */
> -#define HAVE_ARCH_PICK_MMAP_LAYOUT 1
> -
> #ifdef CONFIG_32BIT
> #ifdef CONFIG_KVM_GUEST
> /* User space process size is limited to 1GB in KVM Guest Mode */
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index ffbe69f3a7d9..61e65a69bb09 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -20,43 +20,6 @@
> unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
> EXPORT_SYMBOL(shm_align_mask);
>
> -/* gap between mmap and stack */
> -#define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((STACK_TOP)/6*5)
> -#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
> -
> -static int mmap_is_legacy(struct rlimit *rlim_stack)
> -{
> - if (current->personality & ADDR_COMPAT_LAYOUT)
> - return 1;
> -
> - if (rlim_stack->rlim_cur == RLIM_INFINITY)
> - return 1;
> -
> - return sysctl_legacy_va_layout;
> -}
> -
> -static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> -{
> - unsigned long gap = rlim_stack->rlim_cur;
> - unsigned long pad = stack_guard_gap;
> -
> - /* Account for stack randomization if necessary */
> - if (current->flags & PF_RANDOMIZE)
> - pad += (STACK_RND_MASK << PAGE_SHIFT);
> -
> - /* Values close to RLIM_INFINITY can overflow. */
> - if (gap + pad > gap)
> - gap += pad;
> -
> - if (gap < MIN_GAP)
> - gap = MIN_GAP;
> - else if (gap > MAX_GAP)
> - gap = MAX_GAP;
> -
> - return PAGE_ALIGN(STACK_TOP - gap - rnd);
> -}
> -
> #define COLOUR_ALIGN(addr, pgoff) \
> ((((addr) + shm_align_mask) & ~shm_align_mask) + \
> (((pgoff) << PAGE_SHIFT) & shm_align_mask))
> @@ -154,36 +117,6 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp,
> addr0, len, pgoff, flags, DOWN);
> }
>
> -unsigned long arch_mmap_rnd(void)
> -{
> - unsigned long rnd;
> -
> -#ifdef CONFIG_COMPAT
> - if (TASK_IS_32BIT_ADDR)
> - rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> - else
> -#endif /* CONFIG_COMPAT */
> - rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> -
> - return rnd << PAGE_SHIFT;
> -}
> -
> -void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
> -{
> - unsigned long random_factor = 0UL;
> -
> - if (current->flags & PF_RANDOMIZE)
> - random_factor = arch_mmap_rnd();
> -
> - if (mmap_is_legacy(rlim_stack)) {
> - mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
> - mm->get_unmapped_area = arch_get_unmapped_area;
> - } else {
> - mm->mmap_base = mmap_base(random_factor, rlim_stack);
> - mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> - }
> -}
> -
> static inline unsigned long brk_rnd(void)
> {
> unsigned long rnd = get_random_long();
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:33:51

by Kees Cook

Subject: Re: [PATCH v3 09/11] mips: Use STACK_TOP when computing mmap base address

On Wed, Apr 17, 2019 at 12:32 AM Alexandre Ghiti <[email protected]> wrote:
>
> mmap base address must be computed wrt stack top address, using TASK_SIZE
> is wrong since STACK_TOP and TASK_SIZE are not equivalent.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/mips/mm/mmap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index 3ff82c6f7e24..ffbe69f3a7d9 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -22,7 +22,7 @@ EXPORT_SYMBOL(shm_align_mask);
>
> /* gap between mmap and stack */
> #define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define MAX_GAP ((STACK_TOP)/6*5)
> #define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>
> static int mmap_is_legacy(struct rlimit *rlim_stack)
> @@ -54,7 +54,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> else if (gap > MAX_GAP)
> gap = MAX_GAP;
>
> - return PAGE_ALIGN(TASK_SIZE - gap - rnd);
> + return PAGE_ALIGN(STACK_TOP - gap - rnd);
> }
>
> #define COLOUR_ALIGN(addr, pgoff) \
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:37:09

by Kees Cook

Subject: Re: [PATCH v3 08/11] mips: Properly account for stack randomization and stack guard gap

On Wed, Apr 17, 2019 at 12:31 AM Alexandre Ghiti <[email protected]> wrote:
>
> This commit takes care of stack randomization and stack guard gap when
> computing mmap base address and checks if the task asked for randomization.
> This fixes the problem uncovered and not fixed for mips here:
> https://www.mail-archive.com/[email protected]/msg1429066.html

same URL change here please...

>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/mips/mm/mmap.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index 2f616ebeb7e0..3ff82c6f7e24 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -21,8 +21,9 @@ unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
> EXPORT_SYMBOL(shm_align_mask);
>
> /* gap between mmap and stack */
> -#define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define MIN_GAP (128*1024*1024UL)
> +#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>
> static int mmap_is_legacy(struct rlimit *rlim_stack)
> {
> @@ -38,6 +39,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
> static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> {
> unsigned long gap = rlim_stack->rlim_cur;
> + unsigned long pad = stack_guard_gap;
> +
> + /* Account for stack randomization if necessary */
> + if (current->flags & PF_RANDOMIZE)
> + pad += (STACK_RND_MASK << PAGE_SHIFT);
> +
> + /* Values close to RLIM_INFINITY can overflow. */
> + if (gap + pad > gap)
> + gap += pad;
>
> if (gap < MIN_GAP)
> gap = MIN_GAP;
> --
> 2.20.1
>

--
Kees Cook

2019-04-18 05:41:22

by Kees Cook

Subject: Re: [PATCH v3 11/11] riscv: Make mmap allocation top-down by default

On Wed, Apr 17, 2019 at 12:34 AM Alexandre Ghiti <[email protected]> wrote:
>
> In order to avoid wasting user address space by using bottom-up mmap
> allocation scheme, prefer top-down scheme when possible.
>
> Before:
> root@qemuriscv64:~# cat /proc/self/maps
> 00010000-00016000 r-xp 00000000 fe:00 6389 /bin/cat.coreutils
> 00016000-00017000 r--p 00005000 fe:00 6389 /bin/cat.coreutils
> 00017000-00018000 rw-p 00006000 fe:00 6389 /bin/cat.coreutils
> 00018000-00039000 rw-p 00000000 00:00 0 [heap]
> 1555556000-155556d000 r-xp 00000000 fe:00 7193 /lib/ld-2.28.so
> 155556d000-155556e000 r--p 00016000 fe:00 7193 /lib/ld-2.28.so
> 155556e000-155556f000 rw-p 00017000 fe:00 7193 /lib/ld-2.28.so
> 155556f000-1555570000 rw-p 00000000 00:00 0
> 1555570000-1555572000 r-xp 00000000 00:00 0 [vdso]
> 1555574000-1555576000 rw-p 00000000 00:00 0
> 1555576000-1555674000 r-xp 00000000 fe:00 7187 /lib/libc-2.28.so
> 1555674000-1555678000 r--p 000fd000 fe:00 7187 /lib/libc-2.28.so
> 1555678000-155567a000 rw-p 00101000 fe:00 7187 /lib/libc-2.28.so
> 155567a000-15556a0000 rw-p 00000000 00:00 0
> 3fffb90000-3fffbb1000 rw-p 00000000 00:00 0 [stack]
>
> After:
> root@qemuriscv64:~# cat /proc/self/maps
> 00010000-00016000 r-xp 00000000 fe:00 6389 /bin/cat.coreutils
> 00016000-00017000 r--p 00005000 fe:00 6389 /bin/cat.coreutils
> 00017000-00018000 rw-p 00006000 fe:00 6389 /bin/cat.coreutils
> 00018000-00039000 rw-p 00000000 00:00 0 [heap]
> 3ff7eb6000-3ff7ed8000 rw-p 00000000 00:00 0
> 3ff7ed8000-3ff7fd6000 r-xp 00000000 fe:00 7187 /lib/libc-2.28.so
> 3ff7fd6000-3ff7fda000 r--p 000fd000 fe:00 7187 /lib/libc-2.28.so
> 3ff7fda000-3ff7fdc000 rw-p 00101000 fe:00 7187 /lib/libc-2.28.so
> 3ff7fdc000-3ff7fe2000 rw-p 00000000 00:00 0
> 3ff7fe4000-3ff7fe6000 r-xp 00000000 00:00 0 [vdso]
> 3ff7fe6000-3ff7ffd000 r-xp 00000000 fe:00 7193 /lib/ld-2.28.so
> 3ff7ffd000-3ff7ffe000 r--p 00016000 fe:00 7193 /lib/ld-2.28.so
> 3ff7ffe000-3ff7fff000 rw-p 00017000 fe:00 7193 /lib/ld-2.28.so
> 3ff7fff000-3ff8000000 rw-p 00000000 00:00 0
> 3fff888000-3fff8a9000 rw-p 00000000 00:00 0 [stack]
>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>

Reviewed-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/riscv/Kconfig | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index eb56c82d8aa1..f5897e0dbc1c 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -49,6 +49,17 @@ config RISCV
> select GENERIC_IRQ_MULTI_HANDLER
> select ARCH_HAS_PTE_SPECIAL
> select HAVE_EBPF_JIT if 64BIT
> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
> + select HAVE_ARCH_MMAP_RND_BITS
> +
> +config ARCH_MMAP_RND_BITS_MIN
> + default 18
> +
> +# max bits determined by the following formula:
> +# VA_BITS - PAGE_SHIFT - 3
> +config ARCH_MMAP_RND_BITS_MAX
> + default 33 if 64BIT # SV48 based
> + default 18
>
> config MMU
> def_bool y
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:50:05

by Kees Cook

Subject: Re: [PATCH v3 05/11] arm: Properly account for stack randomization and stack guard gap

On Wed, Apr 17, 2019 at 12:28 AM Alexandre Ghiti <[email protected]> wrote:
>
> This commit takes care of stack randomization and stack guard gap when
> computing mmap base address and checks if the task asked for randomization.
> This fixes the problem uncovered and not fixed for arm here:
> https://www.mail-archive.com/[email protected]/msg1429066.html

Please use the official archive instead. This includes headers, linked
patches, etc:
https://lkml.kernel.org/r/[email protected]

> Signed-off-by: Alexandre Ghiti <[email protected]>
> ---
> arch/arm/mm/mmap.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
> index f866870db749..bff3d00bda5b 100644
> --- a/arch/arm/mm/mmap.c
> +++ b/arch/arm/mm/mmap.c
> @@ -18,8 +18,9 @@
> (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
>
> /* gap between mmap and stack */
> -#define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define MIN_GAP (128*1024*1024UL)

Might as well fix this up as SIZE_128M

> +#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))

STACK_RND_MASK is already defined so you don't need to add it here, yes?

> static int mmap_is_legacy(struct rlimit *rlim_stack)
> {
> @@ -35,6 +36,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
> static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> {
> unsigned long gap = rlim_stack->rlim_cur;
> + unsigned long pad = stack_guard_gap;
> +
> + /* Account for stack randomization if necessary */
> + if (current->flags & PF_RANDOMIZE)
> + pad += (STACK_RND_MASK << PAGE_SHIFT);
> +
> + /* Values close to RLIM_INFINITY can overflow. */
> + if (gap + pad > gap)
> + gap += pad;
>
> if (gap < MIN_GAP)
> gap = MIN_GAP;
> --
> 2.20.1
>

But otherwise, yes:

Acked-by: Kees Cook <[email protected]>

--
Kees Cook

2019-04-18 05:52:41

by Kees Cook

Subject: Re: [PATCH v3 07/11] arm: Use generic mmap top-down layout

On Wed, Apr 17, 2019 at 12:30 AM Alexandre Ghiti <[email protected]> wrote:
>
> arm uses a top-down mmap layout by default that exactly fits the generic
> functions, so get rid of arch specific code and use the generic version
> by selecting ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/arm/Kconfig | 1 +
> arch/arm/include/asm/processor.h | 2 --
> arch/arm/mm/mmap.c | 62 --------------------------------
> 3 files changed, 1 insertion(+), 64 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 850b4805e2d1..f8f603da181f 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -28,6 +28,7 @@ config ARM
> select ARCH_SUPPORTS_ATOMIC_RMW
> select ARCH_USE_BUILTIN_BSWAP
> select ARCH_USE_CMPXCHG_LOCKREF
> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
> select ARCH_WANT_IPC_PARSE_VERSION
> select BUILDTIME_EXTABLE_SORT if MMU
> select CLONE_BACKWARDS
> diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
> index 57fe73ea0f72..944ef1fb1237 100644
> --- a/arch/arm/include/asm/processor.h
> +++ b/arch/arm/include/asm/processor.h
> @@ -143,8 +143,6 @@ static inline void prefetchw(const void *ptr)
> #endif
> #endif
>
> -#define HAVE_ARCH_PICK_MMAP_LAYOUT
> -
> #endif
>
> #endif /* __ASM_ARM_PROCESSOR_H */
> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
> index 0b94b674aa91..b8d912ac9e61 100644
> --- a/arch/arm/mm/mmap.c
> +++ b/arch/arm/mm/mmap.c
> @@ -17,43 +17,6 @@
> ((((addr)+SHMLBA-1)&~(SHMLBA-1)) + \
> (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
>
> -/* gap between mmap and stack */
> -#define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((STACK_TOP)/6*5)
> -#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
> -
> -static int mmap_is_legacy(struct rlimit *rlim_stack)
> -{
> - if (current->personality & ADDR_COMPAT_LAYOUT)
> - return 1;
> -
> - if (rlim_stack->rlim_cur == RLIM_INFINITY)
> - return 1;
> -
> - return sysctl_legacy_va_layout;
> -}
> -
> -static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> -{
> - unsigned long gap = rlim_stack->rlim_cur;
> - unsigned long pad = stack_guard_gap;
> -
> - /* Account for stack randomization if necessary */
> - if (current->flags & PF_RANDOMIZE)
> - pad += (STACK_RND_MASK << PAGE_SHIFT);
> -
> - /* Values close to RLIM_INFINITY can overflow. */
> - if (gap + pad > gap)
> - gap += pad;
> -
> - if (gap < MIN_GAP)
> - gap = MIN_GAP;
> - else if (gap > MAX_GAP)
> - gap = MAX_GAP;
> -
> - return PAGE_ALIGN(STACK_TOP - gap - rnd);
> -}
> -
> /*
> * We need to ensure that shared mappings are correctly aligned to
> * avoid aliasing issues with VIPT caches. We need to ensure that
> @@ -181,31 +144,6 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
> return addr;
> }
>
> -unsigned long arch_mmap_rnd(void)
> -{
> - unsigned long rnd;
> -
> - rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> -
> - return rnd << PAGE_SHIFT;
> -}
> -
> -void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
> -{
> - unsigned long random_factor = 0UL;
> -
> - if (current->flags & PF_RANDOMIZE)
> - random_factor = arch_mmap_rnd();
> -
> - if (mmap_is_legacy(rlim_stack)) {
> - mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
> - mm->get_unmapped_area = arch_get_unmapped_area;
> - } else {
> - mm->mmap_base = mmap_base(random_factor, rlim_stack);
> - mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> - }
> -}
> -
> /*
> * You really shouldn't be using read() or write() on /dev/mem. This
> * might go away in the future.
> --
> 2.20.1
>


--
Kees Cook

2019-04-18 05:56:17

by Alexandre Ghiti

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

On 4/18/19 1:17 AM, Kees Cook wrote:
> (
>
> On Wed, Apr 17, 2019 at 12:27 AM Alexandre Ghiti <[email protected]> wrote:
>> arm64 handles top-down mmap layout in a way that can be easily reused
>> by other architectures, so make it available in mm.
>> It then introduces a new config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
>> that can be set by other architectures to benefit from those functions.
>> Note that this new config depends on MMU being enabled, if selected
>> without MMU support, a warning will be thrown.
>>
>> Suggested-by: Christoph Hellwig <[email protected]>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
>> ---
>> arch/Kconfig | 8 ++++
>> arch/arm64/Kconfig | 1 +
>> arch/arm64/include/asm/processor.h | 2 -
>> arch/arm64/mm/mmap.c | 76 ------------------------------
>> kernel/sysctl.c | 6 ++-
>> mm/util.c | 74 ++++++++++++++++++++++++++++-
>> 6 files changed, 86 insertions(+), 81 deletions(-)
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 33687dddd86a..7c8965c64590 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -684,6 +684,14 @@ config HAVE_ARCH_COMPAT_MMAP_BASES
>> and vice-versa 32-bit applications to call 64-bit mmap().
>> Required for applications doing different bitness syscalls.
>>
>> +# This allows to use a set of generic functions to determine mmap base
>> +# address by giving priority to top-down scheme only if the process
>> +# is not in legacy mode (compat task, unlimited stack size or
>> +# sysctl_legacy_va_layout).
>> +config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
>> + bool
>> + depends on MMU
> I'd prefer the comment were moved to the help text. I would include
> any details about what the arch still needs to define. For example
> right now, I think STACK_RND_MASK is still needed. (Though I think a
> common one could be added for this series too...)


STACK_RND_MASK may be defined by the architecture or it can use the generic
definition in mm/util.c that I moved in patch 1/11 of this series.
That's why I moved
randomize_stack_top in this file.
Regarding the help text, I agree that it does not seem to be frequent to
place
comment above config like that, I'll let Christoph and you decide what's
best. And I'll
add the possibility for the arch to define its own STACK_RND_MASK.


>
>> +
>> config HAVE_COPY_THREAD_TLS
>> bool
>> help
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 7e34b9eba5de..670719a26b45 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -66,6 +66,7 @@ config ARM64
>> select ARCH_SUPPORTS_INT128 if GCC_VERSION >= 50000 || CC_IS_CLANG
>> select ARCH_SUPPORTS_NUMA_BALANCING
>> select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
>> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
>> select ARCH_WANT_FRAME_POINTERS
>> select ARCH_HAS_UBSAN_SANITIZE_ALL
>> select ARM_AMBA
>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> index 5d9ce62bdebd..4de2a2fd605a 100644
>> --- a/arch/arm64/include/asm/processor.h
>> +++ b/arch/arm64/include/asm/processor.h
>> @@ -274,8 +274,6 @@ static inline void spin_lock_prefetch(const void *ptr)
>> "nop") : : "p" (ptr));
>> }
>>
>> -#define HAVE_ARCH_PICK_MMAP_LAYOUT
>> -
>> #endif
>>
>> extern unsigned long __ro_after_init signal_minsigstksz; /* sigframe size */
>> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
>> index ac89686c4af8..c74224421216 100644
>> --- a/arch/arm64/mm/mmap.c
>> +++ b/arch/arm64/mm/mmap.c
>> @@ -31,82 +31,6 @@
>>
>> #include <asm/cputype.h>
>>
>> -/*
>> - * Leave enough space between the mmap area and the stack to honour ulimit in
>> - * the face of randomisation.
>> - */
> This comment goes missing in the move...


True, I should have left it, sorry about that.


>
>> -#define MIN_GAP (SZ_128M)
>> -#define MAX_GAP (STACK_TOP/6*5)
>> -
>> -static int mmap_is_legacy(struct rlimit *rlim_stack)
>> -{
>> - if (current->personality & ADDR_COMPAT_LAYOUT)
>> - return 1;
>> -
>> - if (rlim_stack->rlim_cur == RLIM_INFINITY)
>> - return 1;
>> -
>> - return sysctl_legacy_va_layout;
>> -}
>> -
>> -unsigned long arch_mmap_rnd(void)
>> -{
>> - unsigned long rnd;
>> -
>> -#ifdef CONFIG_COMPAT
>> - if (is_compat_task())
>> - rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
>> - else
>> -#endif
>> - rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
>> - return rnd << PAGE_SHIFT;
>> -}
>> -
>> -static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> -{
>> - unsigned long gap = rlim_stack->rlim_cur;
>> - unsigned long pad = stack_guard_gap;
>> -
>> - /* Account for stack randomization if necessary */
>> - if (current->flags & PF_RANDOMIZE)
>> - pad += (STACK_RND_MASK << PAGE_SHIFT);
>> -
>> - /* Values close to RLIM_INFINITY can overflow. */
>> - if (gap + pad > gap)
>> - gap += pad;
>> -
>> - if (gap < MIN_GAP)
>> - gap = MIN_GAP;
>> - else if (gap > MAX_GAP)
>> - gap = MAX_GAP;
>> -
>> - return PAGE_ALIGN(STACK_TOP - gap - rnd);
>> -}
>> -
>> -/*
>> - * This function, called very early during the creation of a new process VM
>> - * image, sets up which VM layout function to use:
>> - */
>> -void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
>> -{
>> - unsigned long random_factor = 0UL;
>> -
>> - if (current->flags & PF_RANDOMIZE)
>> - random_factor = arch_mmap_rnd();
>> -
>> - /*
>> - * Fall back to the standard layout if the personality bit is set, or
>> - * if the expected stack growth is unlimited:
>> - */
>> - if (mmap_is_legacy(rlim_stack)) {
>> - mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
>> - mm->get_unmapped_area = arch_get_unmapped_area;
>> - } else {
>> - mm->mmap_base = mmap_base(random_factor, rlim_stack);
>> - mm->get_unmapped_area = arch_get_unmapped_area_topdown;
>> - }
>> -}
>> -
>> /*
>> * You really shouldn't be using read() or write() on /dev/mem. This might go
>> * away in the future.
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index e5da394d1ca3..eb3414e78986 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -269,7 +269,8 @@ extern struct ctl_table epoll_table[];
>> extern struct ctl_table firmware_config_table[];
>> #endif
>>
>> -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
>> +#if defined(HAVE_ARCH_PICK_MMAP_LAYOUT) || \
>> + defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
>> int sysctl_legacy_va_layout;
>> #endif
>>
>> @@ -1564,7 +1565,8 @@ static struct ctl_table vm_table[] = {
>> .proc_handler = proc_dointvec,
>> .extra1 = &zero,
>> },
>> -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
>> +#if defined(HAVE_ARCH_PICK_MMAP_LAYOUT) || \
>> + defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
>> {
>> .procname = "legacy_va_layout",
>> .data = &sysctl_legacy_va_layout,
>> diff --git a/mm/util.c b/mm/util.c
>> index a54afb9b4faa..5c3393d32ed1 100644
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -15,7 +15,12 @@
>> #include <linux/vmalloc.h>
>> #include <linux/userfaultfd_k.h>
>> #include <linux/elf.h>
>> +#include <linux/elf-randomize.h>
>> +#include <linux/personality.h>
>> #include <linux/random.h>
>> +#include <linux/processor.h>
>> +#include <linux/sizes.h>
>> +#include <linux/compat.h>
>>
>> #include <linux/uaccess.h>
>>
>> @@ -313,7 +318,74 @@ unsigned long randomize_stack_top(unsigned long stack_top)
>> #endif
>> }
>>
>> -#if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
>> +#ifdef CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
>> +#ifdef CONFIG_ARCH_HAS_ELF_RANDOMIZE
> I think CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT should select
> CONFIG_ARCH_HAS_ELF_RANDOMIZE. It would mean moving


I don't think we should link those 2 features together: an architecture
may want
topdown mmap and don't care about randomization right ?


> arch_randomize_brk() into this patch set too. For arm64 and arm, this
> is totally fine: they have identical logic. On MIPS this would mean
> bumping the randomization up: arm64 uses SZ_32M for 32-bit and SZ_1G
> for 64-bit. MIPS is 8M and 256M respectively. I don't see anything
> that indicates this would be a problem. *cross fingers*
>
> It looks like x86 would need bumping too: it uses 32M on both 32-bit
> and 64-bit. STACK_RND_MASK is the same though.
>
>> +unsigned long arch_mmap_rnd(void)
>> +{
>> + unsigned long rnd;
>> +
>> +#ifdef CONFIG_COMPAT
>> + if (is_compat_task())
>> + rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
>> + else
>> +#endif /* CONFIG_COMPAT */
> The ifdefs on is_compat_task() are not needed: is_compat_task()
> returns 0 in the !CONFIG_COMPAT case.


Actually, I had to add those ifdefs for mmap_rnd_compat_bits, not
is_compat_task.


>
>> + rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
>> +
>> + return rnd << PAGE_SHIFT;
>> +}
>> +#endif /* CONFIG_ARCH_HAS_ELF_RANDOMIZE */
>> +
>> +static int mmap_is_legacy(struct rlimit *rlim_stack)
>> +{
>> + if (current->personality & ADDR_COMPAT_LAYOUT)
>> + return 1;
>> +
>> + if (rlim_stack->rlim_cur == RLIM_INFINITY)
>> + return 1;
>> +
>> + return sysctl_legacy_va_layout;
>> +}
>> +
>> +#define MIN_GAP (SZ_128M)
>> +#define MAX_GAP (STACK_TOP / 6 * 5)
>> +
>> +static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> +{
>> + unsigned long gap = rlim_stack->rlim_cur;
>> + unsigned long pad = stack_guard_gap;
>> +
>> + /* Account for stack randomization if necessary */
>> + if (current->flags & PF_RANDOMIZE)
>> + pad += (STACK_RND_MASK << PAGE_SHIFT);
>> +
>> + /* Values close to RLIM_INFINITY can overflow. */
>> + if (gap + pad > gap)
>> + gap += pad;
>> +
>> + if (gap < MIN_GAP)
>> + gap = MIN_GAP;
>> + else if (gap > MAX_GAP)
>> + gap = MAX_GAP;
>> +
>> + return PAGE_ALIGN(STACK_TOP - gap - rnd);
>> +}
>> +
>> +void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
>> +{
>> + unsigned long random_factor = 0UL;
>> +
>> + if (current->flags & PF_RANDOMIZE)
>> + random_factor = arch_mmap_rnd();
>> +
>> + if (mmap_is_legacy(rlim_stack)) {
>> + mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
>> + mm->get_unmapped_area = arch_get_unmapped_area;
>> + } else {
>> + mm->mmap_base = mmap_base(random_factor, rlim_stack);
>> + mm->get_unmapped_area = arch_get_unmapped_area_topdown;
>> + }
>> +}
>> +#elif defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
>> void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
>> {
>> mm->mmap_base = TASK_UNMAPPED_BASE;
>> --
>> 2.20.1
>>
>
> --
> Kees Cook

2019-04-18 06:02:40

by Alexandre Ghiti

Subject: Re: [PATCH v3 05/11] arm: Properly account for stack randomization and stack guard gap

On 4/18/19 1:26 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:28 AM Alexandre Ghiti <[email protected]> wrote:
>> This commit takes care of stack randomization and stack guard gap when
>> computing mmap base address and checks if the task asked for randomization.
>> This fixes the problem uncovered and not fixed for arm here:
>> https://www.mail-archive.com/[email protected]/msg1429066.html
> Please use the official archive instead. This includes headers, linked
> patches, etc:
> https://lkml.kernel.org/r/[email protected]


Ok, sorry about that, and thanks for the info.


>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
>> ---
>> arch/arm/mm/mmap.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
>> index f866870db749..bff3d00bda5b 100644
>> --- a/arch/arm/mm/mmap.c
>> +++ b/arch/arm/mm/mmap.c
>> @@ -18,8 +18,9 @@
>> (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
>>
>> /* gap between mmap and stack */
>> -#define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define MIN_GAP (128*1024*1024UL)
> Might as well fix this up as SIZE_128M


I left the code as is because it gets removed in the next commit, I did not
even correct the checkpatch warnings. But I can fix that in v4, since there
will be a v4 :)


>
>> +#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
> STACK_RND_MASK is already defined so you don't need to add it here, yes?


At this point, I don't think arm has STACK_RND_MASK defined anywhere since
the generic version is in mm/util.c.


>
>> static int mmap_is_legacy(struct rlimit *rlim_stack)
>> {
>> @@ -35,6 +36,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
>> static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> {
>> unsigned long gap = rlim_stack->rlim_cur;
>> + unsigned long pad = stack_guard_gap;
>> +
>> + /* Account for stack randomization if necessary */
>> + if (current->flags & PF_RANDOMIZE)
>> + pad += (STACK_RND_MASK << PAGE_SHIFT);
>> +
>> + /* Values close to RLIM_INFINITY can overflow. */
>> + if (gap + pad > gap)
>> + gap += pad;
>>
>> if (gap < MIN_GAP)
>> gap = MIN_GAP;
>> --
>> 2.20.1
>>
> But otherwise, yes:
>
> Acked-by: Kees Cook <[email protected]>


Thanks !


>
> --
> Kees Cook
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2019-04-18 06:05:22

by Alexandre Ghiti

Subject: Re: [PATCH v3 06/11] arm: Use STACK_TOP when computing mmap base address

On 4/18/19 1:27 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:29 AM Alexandre Ghiti <[email protected]> wrote:
>> mmap base address must be computed wrt stack top address, using TASK_SIZE
>> is wrong since STACK_TOP and TASK_SIZE are not equivalent.
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
>> ---
>> arch/arm/mm/mmap.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
>> index bff3d00bda5b..0b94b674aa91 100644
>> --- a/arch/arm/mm/mmap.c
>> +++ b/arch/arm/mm/mmap.c
>> @@ -19,7 +19,7 @@
>>
>> /* gap between mmap and stack */
>> #define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define MAX_GAP ((STACK_TOP)/6*5)
> Parens around STACK_TOP aren't needed, but you'll be removing it
> entirely, so I can't complain. ;)
>
>> #define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>>
>> static int mmap_is_legacy(struct rlimit *rlim_stack)
>> @@ -51,7 +51,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> else if (gap > MAX_GAP)
>> gap = MAX_GAP;
>>
>> - return PAGE_ALIGN(TASK_SIZE - gap - rnd);
>> + return PAGE_ALIGN(STACK_TOP - gap - rnd);
>> }
>>
>> /*
>> --
>> 2.20.1
>>
> Acked-by: Kees Cook <[email protected]>


Thanks !

>

2019-04-18 06:07:22

by Alexandre Ghiti

Subject: Re: [PATCH v3 07/11] arm: Use generic mmap top-down layout

On 4/18/19 1:28 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:30 AM Alexandre Ghiti <[email protected]> wrote:
>> arm uses a top-down mmap layout by default that exactly fits the generic
>> functions, so get rid of arch specific code and use the generic version
>> by selecting ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT.
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>


Thanks !


>
> -Kees
>
>> ---
>> arch/arm/Kconfig | 1 +
>> arch/arm/include/asm/processor.h | 2 --
>> arch/arm/mm/mmap.c | 62 --------------------------------
>> 3 files changed, 1 insertion(+), 64 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 850b4805e2d1..f8f603da181f 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -28,6 +28,7 @@ config ARM
>> select ARCH_SUPPORTS_ATOMIC_RMW
>> select ARCH_USE_BUILTIN_BSWAP
>> select ARCH_USE_CMPXCHG_LOCKREF
>> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
>> select ARCH_WANT_IPC_PARSE_VERSION
>> select BUILDTIME_EXTABLE_SORT if MMU
>> select CLONE_BACKWARDS
>> diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
>> index 57fe73ea0f72..944ef1fb1237 100644
>> --- a/arch/arm/include/asm/processor.h
>> +++ b/arch/arm/include/asm/processor.h
>> @@ -143,8 +143,6 @@ static inline void prefetchw(const void *ptr)
>> #endif
>> #endif
>>
>> -#define HAVE_ARCH_PICK_MMAP_LAYOUT
>> -
>> #endif
>>
>> #endif /* __ASM_ARM_PROCESSOR_H */
>> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
>> index 0b94b674aa91..b8d912ac9e61 100644
>> --- a/arch/arm/mm/mmap.c
>> +++ b/arch/arm/mm/mmap.c
>> @@ -17,43 +17,6 @@
>> ((((addr)+SHMLBA-1)&~(SHMLBA-1)) + \
>> (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
>>
>> -/* gap between mmap and stack */
>> -#define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((STACK_TOP)/6*5)
>> -#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>> -
>> -static int mmap_is_legacy(struct rlimit *rlim_stack)
>> -{
>> - if (current->personality & ADDR_COMPAT_LAYOUT)
>> - return 1;
>> -
>> - if (rlim_stack->rlim_cur == RLIM_INFINITY)
>> - return 1;
>> -
>> - return sysctl_legacy_va_layout;
>> -}
>> -
>> -static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> -{
>> - unsigned long gap = rlim_stack->rlim_cur;
>> - unsigned long pad = stack_guard_gap;
>> -
>> - /* Account for stack randomization if necessary */
>> - if (current->flags & PF_RANDOMIZE)
>> - pad += (STACK_RND_MASK << PAGE_SHIFT);
>> -
>> - /* Values close to RLIM_INFINITY can overflow. */
>> - if (gap + pad > gap)
>> - gap += pad;
>> -
>> - if (gap < MIN_GAP)
>> - gap = MIN_GAP;
>> - else if (gap > MAX_GAP)
>> - gap = MAX_GAP;
>> -
>> - return PAGE_ALIGN(STACK_TOP - gap - rnd);
>> -}
>> -
>> /*
>> * We need to ensure that shared mappings are correctly aligned to
>> * avoid aliasing issues with VIPT caches. We need to ensure that
>> @@ -181,31 +144,6 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>> return addr;
>> }
>>
>> -unsigned long arch_mmap_rnd(void)
>> -{
>> - unsigned long rnd;
>> -
>> - rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
>> -
>> - return rnd << PAGE_SHIFT;
>> -}
>> -
>> -void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
>> -{
>> - unsigned long random_factor = 0UL;
>> -
>> - if (current->flags & PF_RANDOMIZE)
>> - random_factor = arch_mmap_rnd();
>> -
>> - if (mmap_is_legacy(rlim_stack)) {
>> - mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
>> - mm->get_unmapped_area = arch_get_unmapped_area;
>> - } else {
>> - mm->mmap_base = mmap_base(random_factor, rlim_stack);
>> - mm->get_unmapped_area = arch_get_unmapped_area_topdown;
>> - }
>> -}
>> -
>> /*
>> * You really shouldn't be using read() or write() on /dev/mem. This
>> * might go away in the future.
>> --
>> 2.20.1
>>
>

2019-04-18 06:07:56

by Alexandre Ghiti

Subject: Re: [PATCH v3 08/11] mips: Properly account for stack randomization and stack guard gap

On 4/18/19 1:30 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:31 AM Alexandre Ghiti <[email protected]> wrote:
>> This commit takes care of stack randomization and stack guard gap when
>> computing mmap base address and checks if the task asked for randomization.
>> This fixes the problem uncovered and not fixed for mips here:
>> https://www.mail-archive.com/[email protected]/msg1429066.html
> same URL change here please...
>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>


Thanks !


>
> -Kees
>
>> ---
>> arch/mips/mm/mmap.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
>> index 2f616ebeb7e0..3ff82c6f7e24 100644
>> --- a/arch/mips/mm/mmap.c
>> +++ b/arch/mips/mm/mmap.c
>> @@ -21,8 +21,9 @@ unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
>> EXPORT_SYMBOL(shm_align_mask);
>>
>> /* gap between mmap and stack */
>> -#define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define MIN_GAP (128*1024*1024UL)
>> +#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>>
>> static int mmap_is_legacy(struct rlimit *rlim_stack)
>> {
>> @@ -38,6 +39,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
>> static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> {
>> unsigned long gap = rlim_stack->rlim_cur;
>> + unsigned long pad = stack_guard_gap;
>> +
>> + /* Account for stack randomization if necessary */
>> + if (current->flags & PF_RANDOMIZE)
>> + pad += (STACK_RND_MASK << PAGE_SHIFT);
>> +
>> + /* Values close to RLIM_INFINITY can overflow. */
>> + if (gap + pad > gap)
>> + gap += pad;
>>
>> if (gap < MIN_GAP)
>> gap = MIN_GAP;
>> --
>> 2.20.1
>>

2019-04-18 06:10:29

by Alexandre Ghiti

Subject: Re: [PATCH v3 11/11] riscv: Make mmap allocation top-down by default

On 4/18/19 1:31 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:34 AM Alexandre Ghiti <[email protected]> wrote:
>> In order to avoid wasting user address space by using bottom-up mmap
>> allocation scheme, prefer top-down scheme when possible.
>>
>> Before:
>> root@qemuriscv64:~# cat /proc/self/maps
>> 00010000-00016000 r-xp 00000000 fe:00 6389 /bin/cat.coreutils
>> 00016000-00017000 r--p 00005000 fe:00 6389 /bin/cat.coreutils
>> 00017000-00018000 rw-p 00006000 fe:00 6389 /bin/cat.coreutils
>> 00018000-00039000 rw-p 00000000 00:00 0 [heap]
>> 1555556000-155556d000 r-xp 00000000 fe:00 7193 /lib/ld-2.28.so
>> 155556d000-155556e000 r--p 00016000 fe:00 7193 /lib/ld-2.28.so
>> 155556e000-155556f000 rw-p 00017000 fe:00 7193 /lib/ld-2.28.so
>> 155556f000-1555570000 rw-p 00000000 00:00 0
>> 1555570000-1555572000 r-xp 00000000 00:00 0 [vdso]
>> 1555574000-1555576000 rw-p 00000000 00:00 0
>> 1555576000-1555674000 r-xp 00000000 fe:00 7187 /lib/libc-2.28.so
>> 1555674000-1555678000 r--p 000fd000 fe:00 7187 /lib/libc-2.28.so
>> 1555678000-155567a000 rw-p 00101000 fe:00 7187 /lib/libc-2.28.so
>> 155567a000-15556a0000 rw-p 00000000 00:00 0
>> 3fffb90000-3fffbb1000 rw-p 00000000 00:00 0 [stack]
>>
>> After:
>> root@qemuriscv64:~# cat /proc/self/maps
>> 00010000-00016000 r-xp 00000000 fe:00 6389 /bin/cat.coreutils
>> 00016000-00017000 r--p 00005000 fe:00 6389 /bin/cat.coreutils
>> 00017000-00018000 rw-p 00006000 fe:00 6389 /bin/cat.coreutils
>> 00018000-00039000 rw-p 00000000 00:00 0 [heap]
>> 3ff7eb6000-3ff7ed8000 rw-p 00000000 00:00 0
>> 3ff7ed8000-3ff7fd6000 r-xp 00000000 fe:00 7187 /lib/libc-2.28.so
>> 3ff7fd6000-3ff7fda000 r--p 000fd000 fe:00 7187 /lib/libc-2.28.so
>> 3ff7fda000-3ff7fdc000 rw-p 00101000 fe:00 7187 /lib/libc-2.28.so
>> 3ff7fdc000-3ff7fe2000 rw-p 00000000 00:00 0
>> 3ff7fe4000-3ff7fe6000 r-xp 00000000 00:00 0 [vdso]
>> 3ff7fe6000-3ff7ffd000 r-xp 00000000 fe:00 7193 /lib/ld-2.28.so
>> 3ff7ffd000-3ff7ffe000 r--p 00016000 fe:00 7193 /lib/ld-2.28.so
>> 3ff7ffe000-3ff7fff000 rw-p 00017000 fe:00 7193 /lib/ld-2.28.so
>> 3ff7fff000-3ff8000000 rw-p 00000000 00:00 0
>> 3fff888000-3fff8a9000 rw-p 00000000 00:00 0 [stack]
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
>> Reviewed-by: Christoph Hellwig <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>


Thank you very much for all your comments,

Alex


>
> -Kees
>
>> ---
>> arch/riscv/Kconfig | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index eb56c82d8aa1..f5897e0dbc1c 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -49,6 +49,17 @@ config RISCV
>> select GENERIC_IRQ_MULTI_HANDLER
>> select ARCH_HAS_PTE_SPECIAL
>> select HAVE_EBPF_JIT if 64BIT
>> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
>> + select HAVE_ARCH_MMAP_RND_BITS
>> +
>> +config ARCH_MMAP_RND_BITS_MIN
>> + default 18
>> +
>> +# max bits determined by the following formula:
>> +# VA_BITS - PAGE_SHIFT - 3
>> +config ARCH_MMAP_RND_BITS_MAX
>> + default 33 if 64BIT # SV48 based
>> + default 18
>>
>> config MMU
>> def_bool y
>> --
>> 2.20.1
>>
>

2019-04-18 06:10:36

by Alexandre Ghiti

Subject: Re: [PATCH v3 09/11] mips: Use STACK_TOP when computing mmap base address

On 4/18/19 1:31 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:32 AM Alexandre Ghiti <[email protected]> wrote:
>> mmap base address must be computed wrt stack top address, using TASK_SIZE
>> is wrong since STACK_TOP and TASK_SIZE are not equivalent.
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>


Thanks !


>
> -Kees
>
>> ---
>> arch/mips/mm/mmap.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
>> index 3ff82c6f7e24..ffbe69f3a7d9 100644
>> --- a/arch/mips/mm/mmap.c
>> +++ b/arch/mips/mm/mmap.c
>> @@ -22,7 +22,7 @@ EXPORT_SYMBOL(shm_align_mask);
>>
>> /* gap between mmap and stack */
>> #define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define MAX_GAP ((STACK_TOP)/6*5)
>> #define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>>
>> static int mmap_is_legacy(struct rlimit *rlim_stack)
>> @@ -54,7 +54,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> else if (gap > MAX_GAP)
>> gap = MAX_GAP;
>>
>> - return PAGE_ALIGN(TASK_SIZE - gap - rnd);
>> + return PAGE_ALIGN(STACK_TOP - gap - rnd);
>> }
>>
>> #define COLOUR_ALIGN(addr, pgoff) \
>> --
>> 2.20.1
>>
>

2019-04-18 06:11:08

by Alexandre Ghiti

Subject: Re: [PATCH v3 10/11] mips: Use generic mmap top-down layout

On 4/18/19 1:31 AM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 12:33 AM Alexandre Ghiti <[email protected]> wrote:
>> mips uses a top-down layout by default that fits the generic functions.
>> At the same time, this commit allows to fix problem uncovered
>> and not fixed for mips here:
>> https://www.mail-archive.com/[email protected]/msg1429066.html
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>

Thanks !


>
> -Kees
>
>> ---
>> arch/mips/Kconfig | 1 +
>> arch/mips/include/asm/processor.h | 5 ---
>> arch/mips/mm/mmap.c | 67 -------------------------------
>> 3 files changed, 1 insertion(+), 72 deletions(-)
>>
>> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
>> index 4a5f5b0ee9a9..ec2f07561e4d 100644
>> --- a/arch/mips/Kconfig
>> +++ b/arch/mips/Kconfig
>> @@ -14,6 +14,7 @@ config MIPS
>> select ARCH_USE_CMPXCHG_LOCKREF if 64BIT
>> select ARCH_USE_QUEUED_RWLOCKS
>> select ARCH_USE_QUEUED_SPINLOCKS
>> + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
>> select ARCH_WANT_IPC_PARSE_VERSION
>> select BUILDTIME_EXTABLE_SORT
>> select CLONE_BACKWARDS
>> diff --git a/arch/mips/include/asm/processor.h b/arch/mips/include/asm/processor.h
>> index aca909bd7841..fba18d4a9190 100644
>> --- a/arch/mips/include/asm/processor.h
>> +++ b/arch/mips/include/asm/processor.h
>> @@ -29,11 +29,6 @@
>>
>> extern unsigned int vced_count, vcei_count;
>>
>> -/*
>> - * MIPS does have an arch_pick_mmap_layout()
>> - */
>> -#define HAVE_ARCH_PICK_MMAP_LAYOUT 1
>> -
>> #ifdef CONFIG_32BIT
>> #ifdef CONFIG_KVM_GUEST
>> /* User space process size is limited to 1GB in KVM Guest Mode */
>> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
>> index ffbe69f3a7d9..61e65a69bb09 100644
>> --- a/arch/mips/mm/mmap.c
>> +++ b/arch/mips/mm/mmap.c
>> @@ -20,43 +20,6 @@
>> unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
>> EXPORT_SYMBOL(shm_align_mask);
>>
>> -/* gap between mmap and stack */
>> -#define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((STACK_TOP)/6*5)
>> -#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>> -
>> -static int mmap_is_legacy(struct rlimit *rlim_stack)
>> -{
>> - if (current->personality & ADDR_COMPAT_LAYOUT)
>> - return 1;
>> -
>> - if (rlim_stack->rlim_cur == RLIM_INFINITY)
>> - return 1;
>> -
>> - return sysctl_legacy_va_layout;
>> -}
>> -
>> -static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> -{
>> - unsigned long gap = rlim_stack->rlim_cur;
>> - unsigned long pad = stack_guard_gap;
>> -
>> - /* Account for stack randomization if necessary */
>> - if (current->flags & PF_RANDOMIZE)
>> - pad += (STACK_RND_MASK << PAGE_SHIFT);
>> -
>> - /* Values close to RLIM_INFINITY can overflow. */
>> - if (gap + pad > gap)
>> - gap += pad;
>> -
>> - if (gap < MIN_GAP)
>> - gap = MIN_GAP;
>> - else if (gap > MAX_GAP)
>> - gap = MAX_GAP;
>> -
>> - return PAGE_ALIGN(STACK_TOP - gap - rnd);
>> -}
>> -
>> #define COLOUR_ALIGN(addr, pgoff) \
>> ((((addr) + shm_align_mask) & ~shm_align_mask) + \
>> (((pgoff) << PAGE_SHIFT) & shm_align_mask))
>> @@ -154,36 +117,6 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp,
>> addr0, len, pgoff, flags, DOWN);
>> }
>>
>> -unsigned long arch_mmap_rnd(void)
>> -{
>> - unsigned long rnd;
>> -
>> -#ifdef CONFIG_COMPAT
>> - if (TASK_IS_32BIT_ADDR)
>> - rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
>> - else
>> -#endif /* CONFIG_COMPAT */
>> - rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
>> -
>> - return rnd << PAGE_SHIFT;
>> -}
>> -
>> -void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
>> -{
>> - unsigned long random_factor = 0UL;
>> -
>> - if (current->flags & PF_RANDOMIZE)
>> - random_factor = arch_mmap_rnd();
>> -
>> - if (mmap_is_legacy(rlim_stack)) {
>> - mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
>> - mm->get_unmapped_area = arch_get_unmapped_area;
>> - } else {
>> - mm->mmap_base = mmap_base(random_factor, rlim_stack);
>> - mm->get_unmapped_area = arch_get_unmapped_area_topdown;
>> - }
>> -}
>> -
>> static inline unsigned long brk_rnd(void)
>> {
>> unsigned long rnd = get_random_long();
>> --
>> 2.20.1
>>
>

2019-04-18 14:21:05

by Kees Cook

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <[email protected]> wrote:
> Regarding the help text, I agree that it does not seem to be frequent to
> place
> comment above config like that, I'll let Christoph and you decide what's
> best. And I'll
> add the possibility for the arch to define its own STACK_RND_MASK.

Yeah, I think it's very helpful to spell out the requirements for new
architectures with these kinds of features in the help text (see
SECCOMP_FILTER for example).

> > I think CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT should select
> > CONFIG_ARCH_HAS_ELF_RANDOMIZE. It would mean moving
>
>
> I don't think we should link those 2 features together: an architecture
> may want
> topdown mmap and don't care about randomization right ?

Given that the mmap randomization and stack randomization are already
coming along for the ride, it seems weird to make brk randomization an
optional feature (especially since all the of the architectures you're
converting include it). I'd also like these kinds of security features
to be available by default. So, I think one patch to adjust the MIPS
brk randomization entropy and then you can just include it in this
move.

> Actually, I had to add those ifdefs for mmap_rnd_compat_bits, not
> is_compat_task.

Oh! In that case, use CONFIG_HAVE_ARCH_MMAP_RND_BITS. :) Actually,
what would be maybe cleaner would be to add mmap_rnd_bits_min/max
consts set to 0 for the non-CONFIG_HAVE_ARCH_MMAP_RND_BITS case at the
top of mm/mmap.c.

I really like this clean-up! I think we can move x86 to it too without
too much pain. :)

--
Kees Cook

2019-04-18 21:28:25

by Paul Burton

Subject: Re: [PATCH v3 08/11] mips: Properly account for stack randomization and stack guard gap

Hi Alexandre,

On Wed, Apr 17, 2019 at 01:22:44AM -0400, Alexandre Ghiti wrote:
> This commit takes care of stack randomization and stack guard gap when
> computing mmap base address and checks if the task asked for randomization.
> This fixes the problem uncovered and not fixed for mips here:
> https://www.mail-archive.com/[email protected]/msg1429066.html
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

For patches 8-10:

Acked-by: Paul Burton <[email protected]>

Thanks for improving this,

Paul

> ---
> arch/mips/mm/mmap.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index 2f616ebeb7e0..3ff82c6f7e24 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -21,8 +21,9 @@ unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
> EXPORT_SYMBOL(shm_align_mask);
>
> /* gap between mmap and stack */
> -#define MIN_GAP (128*1024*1024UL)
> -#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define MIN_GAP (128*1024*1024UL)
> +#define MAX_GAP ((TASK_SIZE)/6*5)
> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>
> static int mmap_is_legacy(struct rlimit *rlim_stack)
> {
> @@ -38,6 +39,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
> static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
> {
> unsigned long gap = rlim_stack->rlim_cur;
> + unsigned long pad = stack_guard_gap;
> +
> + /* Account for stack randomization if necessary */
> + if (current->flags & PF_RANDOMIZE)
> + pad += (STACK_RND_MASK << PAGE_SHIFT);
> +
> + /* Values close to RLIM_INFINITY can overflow. */
> + if (gap + pad > gap)
> + gap += pad;
>
> if (gap < MIN_GAP)
> gap = MIN_GAP;
> --
> 2.20.1
>

2019-04-19 19:02:49

by Alexandre Ghiti

Subject: Re: [PATCH v3 08/11] mips: Properly account for stack randomization and stack guard gap

On 4/18/19 5:27 PM, Paul Burton wrote:
> Hi Alexandre,
>
> On Wed, Apr 17, 2019 at 01:22:44AM -0400, Alexandre Ghiti wrote:
>> This commit takes care of stack randomization and stack guard gap when
>> computing mmap base address and checks if the task asked for randomization.
>> This fixes the problem uncovered and not fixed for mips here:
>> https://www.mail-archive.com/[email protected]/msg1429066.html
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> For patches 8-10:
>
> Acked-by: Paul Burton <[email protected]>
>
> Thanks for improving this,


Thank you for your time,


Alex


> Paul
>
>> ---
>> arch/mips/mm/mmap.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
>> index 2f616ebeb7e0..3ff82c6f7e24 100644
>> --- a/arch/mips/mm/mmap.c
>> +++ b/arch/mips/mm/mmap.c
>> @@ -21,8 +21,9 @@ unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
>> EXPORT_SYMBOL(shm_align_mask);
>>
>> /* gap between mmap and stack */
>> -#define MIN_GAP (128*1024*1024UL)
>> -#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define MIN_GAP (128*1024*1024UL)
>> +#define MAX_GAP ((TASK_SIZE)/6*5)
>> +#define STACK_RND_MASK (0x7ff >> (PAGE_SHIFT - 12))
>>
>> static int mmap_is_legacy(struct rlimit *rlim_stack)
>> {
>> @@ -38,6 +39,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
>> static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>> {
>> unsigned long gap = rlim_stack->rlim_cur;
>> + unsigned long pad = stack_guard_gap;
>> +
>> + /* Account for stack randomization if necessary */
>> + if (current->flags & PF_RANDOMIZE)
>> + pad += (STACK_RND_MASK << PAGE_SHIFT);
>> +
>> + /* Values close to RLIM_INFINITY can overflow. */
>> + if (gap + pad > gap)
>> + gap += pad;
>>
>> if (gap < MIN_GAP)
>> gap = MIN_GAP;
>> --
>> 2.20.1
>>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2019-04-19 20:07:58

by Alexandre Ghiti

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

On 4/18/19 10:19 AM, Kees Cook wrote:
> On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <[email protected]> wrote:
>> Regarding the help text, I agree that it does not seem to be frequent to
>> place
>> comment above config like that, I'll let Christoph and you decide what's
>> best. And I'll
>> add the possibility for the arch to define its own STACK_RND_MASK.
> Yeah, I think it's very helpful to spell out the requirements for new
> architectures with these kinds of features in the help text (see
> SECCOMP_FILTER for example).
>
>>> I think CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT should select
>>> CONFIG_ARCH_HAS_ELF_RANDOMIZE. It would mean moving
>>
>> I don't think we should link those 2 features together: an architecture
>> may want
>> topdown mmap and don't care about randomization right ?
> Given that the mmap randomization and stack randomization are already
> coming along for the ride, it seems weird to make brk randomization an
> optional feature (especially since all the of the architectures you're
> converting include it). I'd also like these kinds of security features
> to be available by default. So, I think one patch to adjust the MIPS
> brk randomization entropy and then you can just include it in this
> move.


Ok that makes sense, and that would bring support for randomization to
riscv at the same time, so I'll look into it, thanks.


>> Actually, I had to add those ifdefs for mmap_rnd_compat_bits, not
>> is_compat_task.
> Oh! In that case, use CONFIG_HAVE_ARCH_MMAP_RND_BITS. :) Actually,
> what would be maybe cleaner would be to add mmap_rnd_bits_min/max
> consts set to 0 for the non-CONFIG_HAVE_ARCH_MMAP_RND_BITS case at the
> top of mm/mmap.c.


Ok I'll do that.


>
> I really like this clean-up! I think we can move x86 to it too without
> too much pain. :)
>

Yeah I think too, I will do that too.


Thanks again,


Alex

2019-04-22 19:56:10

by Christoph Hellwig

Subject: Re: [PATCH v3 02/11] arm64: Make use of is_compat_task instead of hardcoding this test

On Wed, Apr 17, 2019 at 01:22:38AM -0400, Alexandre Ghiti wrote:
> Each architecture has its own way to determine if a task is a compat task,
> by using is_compat_task in arch_mmap_rnd, it allows more genericity and
> then it prepares its moving to mm/.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2019-04-22 23:14:32

by Christoph Hellwig

Subject: Re: [PATCH v3 03/11] arm64: Consider stack randomization for mmap base only when necessary

On Wed, Apr 17, 2019 at 01:22:39AM -0400, Alexandre Ghiti wrote:
> Do not offset mmap base address because of stack randomization if
> current task does not want randomization.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2019-04-22 23:18:59

by Christoph Hellwig

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

On Wed, Apr 17, 2019 at 01:22:40AM -0400, Alexandre Ghiti wrote:
> arm64 handles top-down mmap layout in a way that can be easily reused
> by other architectures, so make it available in mm.
> It then introduces a new config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> that can be set by other architectures to benefit from those functions.
> Note that this new config depends on MMU being enabled, if selected
> without MMU support, a warning will be thrown.
>
> Suggested-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Alexandre Ghiti <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2019-04-22 23:28:35

by Christoph Hellwig

Subject: Re: [PATCH v3 11/11] riscv: Make mmap allocation top-down by default

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2019-04-23 02:14:51

by Christoph Hellwig

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

On Thu, Apr 18, 2019 at 09:19:18AM -0500, Kees Cook wrote:
> On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <[email protected]> wrote:
> > Regarding the help text, I agree that it does not seem to be frequent to
> > place
> > comment above config like that, I'll let Christoph and you decide what's
> > best. And I'll
> > add the possibility for the arch to define its own STACK_RND_MASK.
>
> Yeah, I think it's very helpful to spell out the requirements for new
> architectures with these kinds of features in the help text (see
> SECCOMP_FILTER for example).

Spelling out the requirements sounds useful. Abusing the help text
for an option for which no help text can be displayed it pointless.
Just make it a comment as Alex did in this patch, which makes whole
lot more sense.

> > Actually, I had to add those ifdefs for mmap_rnd_compat_bits, not
> > is_compat_task.
>
> Oh! In that case, use CONFIG_HAVE_ARCH_MMAP_RND_BITS. :) Actually,
> what would be maybe cleaner would be to add mmap_rnd_bits_min/max
> consts set to 0 for the non-CONFIG_HAVE_ARCH_MMAP_RND_BITS case at the
> top of mm/mmap.c.

Lets do that in a second step. The current series is already big
enough and a major improvement, even if there is much more to clean
up in this area still left.

2019-04-28 14:29:04

by Alexandre Ghiti

Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

On 4/18/19 10:19 AM, Kees Cook wrote:
> On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <[email protected]> wrote:
>> Regarding the help text, I agree that it does not seem to be frequent to
>> place
>> comment above config like that, I'll let Christoph and you decide what's
>> best. And I'll
>> add the possibility for the arch to define its own STACK_RND_MASK.
> Yeah, I think it's very helpful to spell out the requirements for new
> architectures with these kinds of features in the help text (see
> SECCOMP_FILTER for example).
>
>>> I think CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT should select
>>> CONFIG_ARCH_HAS_ELF_RANDOMIZE. It would mean moving
>>
>> I don't think we should link those 2 features together: an architecture
>> may want
>> topdown mmap and don't care about randomization right ?
> Given that the mmap randomization and stack randomization are already
> coming along for the ride, it seems weird to make brk randomization an
> optional feature (especially since all the of the architectures you're
> converting include it). I'd also like these kinds of security features
> to be available by default. So, I think one patch to adjust the MIPS
> brk randomization entropy and then you can just include it in this
> move.
>
>> Actually, I had to add those ifdefs for mmap_rnd_compat_bits, not
>> is_compat_task.
> Oh! In that case, use CONFIG_HAVE_ARCH_MMAP_RND_BITS. :) Actually,
> what would be maybe cleaner would be to add mmap_rnd_bits_min/max
> consts set to 0 for the non-CONFIG_HAVE_ARCH_MMAP_RND_BITS case at the
> top of mm/mmap.c.
>
> I really like this clean-up! I think we can move x86 to it too without
> too much pain. :)
>
Hi,

Just a short note to indicate that while working on v4, I realized this
series had a some issues:

- I broke the case ARCH_HAS_ELF_RANDOMIZE selected but not
? ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT (which can happen on arm
without MMU for example)
- I use mmap_rnd_bits unconditionnally whereas it might not be defined
(it works for all arches I moved though)

The only clean solution I found for the first problem is to propose a
common implementation for arch_randomize_brk
and arch_mmap_rnd, which is another series on its own and another good
cleanup since every architecture uses
the same functions (except ppc, but that can be workarounded easily).
Just like moving x86 deserves its own series since it adds up 8/9 commits.
I am on vacations for 2 weeks, so I won't have time to submit another
patchset before, sorry about that.

Thanks,

Alex