2019-05-26 13:49:41

by Alexandre Ghiti

Subject: [PATCH v4 00/14] 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 v4:
- Make ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select ARCH_HAS_ELF_RANDOMIZE
by default as suggested by Kees,
- ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT depends on MMU and defines the
functions needed by ARCH_HAS_ELF_RANDOMIZE => architectures that use
the generic mmap topdown functions cannot have ARCH_HAS_ELF_RANDOMIZE
selected without MMU, but I think it's ok since randomization without
MMU does not add much security anyway.
- There is no common API to determine if a process is 32b, so I came up with
!IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14].
- Mention in the change log that x86 already takes care of not offseting mmap
base address if the task does not want randomization.
- Re-introduce a comment that should not have been removed.
- Add Reviewed/Acked-By from Paul, Christoph and Kees, thank you for that.
- I tried to minimize the changes from the commits in v3 in order to make
easier the review of the v4, the commits changed or added are:
- [PATCH v4 5/14]
- [PATCH v4 8/14]
- [PATCH v4 11/14]
- [PATCH v4 12/14]
- [PATCH v4 13/14]

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 (14):
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
arm64, mm: Make randomization selected by generic topdown mmap layout
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 and brk randomization
mips: Properly account for stack randomization and stack guard gap
mips: Use STACK_TOP when computing mmap base address
mips: Adjust brk randomization offset to fit generic version
mips: Replace arch specific way to determine 32bit task with generic
version
mips: Use generic mmap top-down layout and brk randomization
riscv: Make mmap allocation top-down by default

arch/Kconfig | 11 +++
arch/arm/Kconfig | 2 +-
arch/arm/include/asm/processor.h | 2 -
arch/arm/kernel/process.c | 5 --
arch/arm/mm/mmap.c | 52 --------------
arch/arm64/Kconfig | 2 +-
arch/arm64/include/asm/processor.h | 2 -
arch/arm64/kernel/process.c | 8 ---
arch/arm64/mm/mmap.c | 72 -------------------
arch/mips/Kconfig | 2 +-
arch/mips/include/asm/processor.h | 5 --
arch/mips/mm/mmap.c | 84 ----------------------
arch/riscv/Kconfig | 11 +++
fs/binfmt_elf.c | 20 ------
include/linux/mm.h | 2 +
kernel/sysctl.c | 6 +-
mm/util.c | 107 ++++++++++++++++++++++++++++-
17 files changed, 137 insertions(+), 256 deletions(-)

--
2.20.1

2019-05-26 13:51:52

by Alexandre Ghiti

Subject: [PATCH v4 01/14] 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]>
Acked-by: Kees Cook <[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 fa9e99a962e0..d4d2fe109ee9 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -669,26 +669,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 0e8834ac32b7..446ec32c62b8 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2368,6 +2368,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 e2e4f8c3fa12..dab33b896146 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-05-26 13:52:02

by Alexandre Ghiti

Subject: [PATCH v4 02/14] 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]>
Acked-by: Kees Cook <[email protected]>
Reviewed-by: Christoph Hellwig <[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-05-26 13:54:27

by Alexandre Ghiti

Subject: [PATCH v4 03/14] 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.
Note that x86 already implements this behaviour.

Signed-off-by: Alexandre Ghiti <[email protected]>
Acked-by: Kees Cook <[email protected]>
Reviewed-by: Christoph Hellwig <[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-05-26 13:55:10

by Alexandre Ghiti

Subject: [PATCH v4 05/14] arm64, mm: Make randomization selected by generic topdown mmap layout

This commits selects ARCH_HAS_ELF_RANDOMIZE when an arch uses the generic
topdown mmap layout functions so that this security feature is on by
default.
Note that this commit also removes the possibility for arm64 to have elf
randomization and no MMU: without MMU, the security added by randomization
is worth nothing.

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/Kconfig | 1 +
arch/arm64/Kconfig | 1 -
arch/arm64/kernel/process.c | 8 --------
mm/util.c | 11 +++++++++--
4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index df3ab04270fa..3732654446cc 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -710,6 +710,7 @@ config HAVE_ARCH_COMPAT_MMAP_BASES
config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
bool
depends on MMU
+ select ARCH_HAS_ELF_RANDOMIZE

config HAVE_COPY_THREAD_TLS
bool
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3d754c19c11e..403bd3fffdbc 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -15,7 +15,6 @@ config ARM64
select ARCH_HAS_DMA_MMAP_PGPROT
select ARCH_HAS_DMA_PREP_COHERENT
select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
- select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FAST_MULTIPLIER
select ARCH_HAS_FORTIFY_SOURCE
select ARCH_HAS_GCOV_PROFILE_ALL
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 3767fb21a5b8..3f85f8f2d665 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -535,14 +535,6 @@ unsigned long arch_align_stack(unsigned long sp)
return sp & ~0xf;
}

-unsigned long arch_randomize_brk(struct mm_struct *mm)
-{
- if (is_compat_task())
- return randomize_page(mm->brk, SZ_32M);
- else
- return randomize_page(mm->brk, SZ_1G);
-}
-
/*
* Called from setup_new_exec() after (COMPAT_)SET_PERSONALITY.
*/
diff --git a/mm/util.c b/mm/util.c
index 717f5d75c16e..8a38126edc74 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -319,7 +319,15 @@ unsigned long randomize_stack_top(unsigned long stack_top)
}

#ifdef CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
-#ifdef CONFIG_ARCH_HAS_ELF_RANDOMIZE
+unsigned long arch_randomize_brk(struct mm_struct *mm)
+{
+ /* Is the current task 32bit ? */
+ if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task())
+ return randomize_page(mm->brk, SZ_32M);
+
+ return randomize_page(mm->brk, SZ_1G);
+}
+
unsigned long arch_mmap_rnd(void)
{
unsigned long rnd;
@@ -333,7 +341,6 @@ unsigned long arch_mmap_rnd(void)

return rnd << PAGE_SHIFT;
}
-#endif /* CONFIG_ARCH_HAS_ELF_RANDOMIZE */

static int mmap_is_legacy(struct rlimit *rlim_stack)
{
--
2.20.1

2019-05-26 13:56:17

by Alexandre Ghiti

Subject: [PATCH v4 06/14] 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://lkml.kernel.org/r/[email protected]

Signed-off-by: Alexandre Ghiti <[email protected]>
Acked-by: Kees Cook <[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-05-26 13:58:35

by Alexandre Ghiti

Subject: [PATCH v4 08/14] arm: Use generic mmap top-down layout and brk randomization

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.
As ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT selects ARCH_HAS_ELF_RANDOMIZE,
use the generic version of arch_randomize_brk since it also fits.
Note that this commit also removes the possibility for arm to have elf
randomization and no MMU: without MMU, the security added by randomization
is worth nothing.

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

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 8869742a85df..27687a8c9fb5 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -6,7 +6,6 @@ config ARM
select ARCH_CLOCKSOURCE_DATA
select ARCH_HAS_DEBUG_VIRTUAL if MMU
select ARCH_HAS_DEVMEM_IS_ALLOWED
- select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
select ARCH_HAS_KEEPINITRD
select ARCH_HAS_KCOV
@@ -29,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 5d06f75ffad4..95b7688341c5 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/kernel/process.c b/arch/arm/kernel/process.c
index 72cc0862a30e..19a765db5f7f 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -322,11 +322,6 @@ unsigned long get_wchan(struct task_struct *p)
return 0;
}

-unsigned long arch_randomize_brk(struct mm_struct *mm)
-{
- return randomize_page(mm->brk, 0x02000000);
-}
-
#ifdef CONFIG_MMU
#ifdef CONFIG_KUSER_HELPERS
/*
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-05-26 13:58:43

by Alexandre Ghiti

Subject: [PATCH v4 07/14] 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]>
Acked-by: Kees Cook <[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-05-26 14:01:02

by Alexandre Ghiti

Subject: [PATCH v4 10/14] 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]>
Acked-by: Kees Cook <[email protected]>
Acked-by: Paul Burton <[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-05-26 14:01:03

by Alexandre Ghiti

Subject: [PATCH v4 09/14] 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 arm here:
https://lkml.kernel.org/r/[email protected]

Signed-off-by: Alexandre Ghiti <[email protected]>
Acked-by: Kees Cook <[email protected]>
Acked-by: Paul Burton <[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-05-26 14:01:49

by Alexandre Ghiti

Subject: [PATCH v4 11/14] mips: Adjust brk randomization offset to fit generic version

This commit simply bumps up to 32MB and 1GB the random offset
of brk, compared to 8MB and 256MB, for 32bit and 64bit respectively.

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

diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index ffbe69f3a7d9..c052565b76fb 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -16,6 +16,7 @@
#include <linux/random.h>
#include <linux/sched/signal.h>
#include <linux/sched/mm.h>
+#include <linux/sizes.h>

unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
EXPORT_SYMBOL(shm_align_mask);
@@ -189,11 +190,11 @@ static inline unsigned long brk_rnd(void)
unsigned long rnd = get_random_long();

rnd = rnd << PAGE_SHIFT;
- /* 8MB for 32bit, 256MB for 64bit */
+ /* 32MB for 32bit, 1GB for 64bit */
if (TASK_IS_32BIT_ADDR)
- rnd = rnd & 0x7ffffful;
+ rnd = rnd & SZ_32M;
else
- rnd = rnd & 0xffffffful;
+ rnd = rnd & SZ_1G;

return rnd;
}
--
2.20.1

2019-05-26 14:02:57

by Alexandre Ghiti

Subject: [PATCH v4 12/14] mips: Replace arch specific way to determine 32bit task with generic version

Mips uses TASK_IS_32BIT_ADDR to determine if a task is 32bit, but
this define is mips specific and other arches do not have it: instead,
use !IS_ENABLED(CONFIG_64BIT) || is_compat_task() condition.

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

diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index c052565b76fb..900670ea8531 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -17,6 +17,7 @@
#include <linux/sched/signal.h>
#include <linux/sched/mm.h>
#include <linux/sizes.h>
+#include <linux/compat.h>

unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
EXPORT_SYMBOL(shm_align_mask);
@@ -191,7 +192,7 @@ static inline unsigned long brk_rnd(void)

rnd = rnd << PAGE_SHIFT;
/* 32MB for 32bit, 1GB for 64bit */
- if (TASK_IS_32BIT_ADDR)
+ if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task())
rnd = rnd & SZ_32M;
else
rnd = rnd & SZ_1G;
--
2.20.1

2019-05-26 14:03:56

by Alexandre Ghiti

Subject: [PATCH v4 13/14] mips: Use generic mmap top-down layout and brk randomization

mips uses a top-down 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.
As ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT selects ARCH_HAS_ELF_RANDOMIZE,
use the generic version of arch_randomize_brk since it also fits.
Note that this commit also removes the possibility for mips to have elf
randomization and no MMU: without MMU, the security added by randomization
is worth nothing.

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

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 70d3200476bf..da15b02bbe23 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -5,7 +5,6 @@ config MIPS
select ARCH_32BIT_OFF_T if !64BIT
select ARCH_BINFMT_ELF_STATE if MIPS_FP_SUPPORT
select ARCH_CLOCKSOURCE_DATA
- select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UBSAN_SANITIZE_ALL
select ARCH_SUPPORTS_UPROBES
@@ -13,6 +12,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 900670ea8531..c2effe535484 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -16,49 +16,10 @@
#include <linux/random.h>
#include <linux/sched/signal.h>
#include <linux/sched/mm.h>
-#include <linux/sizes.h>
-#include <linux/compat.h>

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))
@@ -156,63 +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();
-
- rnd = rnd << PAGE_SHIFT;
- /* 32MB for 32bit, 1GB for 64bit */
- if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task())
- rnd = rnd & SZ_32M;
- else
- rnd = rnd & SZ_1G;
-
- return rnd;
-}
-
-unsigned long arch_randomize_brk(struct mm_struct *mm)
-{
- unsigned long base = mm->brk;
- unsigned long ret;
-
- ret = PAGE_ALIGN(base + brk_rnd());
-
- if (ret < mm->brk)
- return mm->brk;
-
- return ret;
-}
-
int __virt_addr_valid(const volatile void *kaddr)
{
return pfn_valid(PFN_DOWN(virt_to_phys(kaddr)));
--
2.20.1

2019-05-26 14:05:03

by Alexandre Ghiti

Subject: [PATCH v4 14/14] 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
2de81000-2dea2000 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]>
---
arch/riscv/Kconfig | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index ee32c66e1af3..8c067ebd3ae0 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -49,6 +49,17 @@ config RISCV
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_MMIOWB
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-05-26 14:19:55

by Alexandre Ghiti

Subject: [PATCH v4 04/14] 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]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
arch/Kconfig | 10 ++++
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/processor.h | 2 -
arch/arm64/mm/mmap.c | 76 -----------------------------
kernel/sysctl.c | 6 ++-
mm/util.c | 78 +++++++++++++++++++++++++++++-
6 files changed, 92 insertions(+), 81 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index c47b328eada0..df3ab04270fa 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -701,6 +701,16 @@ 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).
+# Architecture that selects this option can provide its own version of:
+# - STACK_RND_MASK
+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 4780eb7af842..3d754c19c11e 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -69,6 +69,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 fcd0e691b1ea..3bd818edf319 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -282,8 +282,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 943c89178e3d..aebd03cc4b65 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -271,7 +271,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

@@ -1566,7 +1567,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 dab33b896146..717f5d75c16e 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,78 @@ 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_HAVE_ARCH_MMAP_RND_COMPAT_BITS
+ if (is_compat_task())
+ rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
+ else
+#endif /* CONFIG_HAVE_ARCH_MMAP_RND_COMPAT_BITS */
+ 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;
+}
+
+/*
+ * 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 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-05-29 19:28:59

by Kees Cook

Subject: Re: [PATCH v4 08/14] arm: Use generic mmap top-down layout and brk randomization

On Sun, May 26, 2019 at 09:47:40AM -0400, Alexandre Ghiti 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.
> As ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT selects ARCH_HAS_ELF_RANDOMIZE,
> use the generic version of arch_randomize_brk since it also fits.
> Note that this commit also removes the possibility for arm to have elf
> randomization and no MMU: without MMU, the security added by randomization
> is worth nothing.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

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

It may be worth noting that STACK_RND_MASK is safe to remove here
because it matches the default that now exists in mm/util.c.

-Kees

> ---
> arch/arm/Kconfig | 2 +-
> arch/arm/include/asm/processor.h | 2 --
> arch/arm/kernel/process.c | 5 ---
> arch/arm/mm/mmap.c | 62 --------------------------------
> 4 files changed, 1 insertion(+), 70 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 8869742a85df..27687a8c9fb5 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -6,7 +6,6 @@ config ARM
> select ARCH_CLOCKSOURCE_DATA
> select ARCH_HAS_DEBUG_VIRTUAL if MMU
> select ARCH_HAS_DEVMEM_IS_ALLOWED
> - select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_FORTIFY_SOURCE
> select ARCH_HAS_KEEPINITRD
> select ARCH_HAS_KCOV
> @@ -29,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 5d06f75ffad4..95b7688341c5 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/kernel/process.c b/arch/arm/kernel/process.c
> index 72cc0862a30e..19a765db5f7f 100644
> --- a/arch/arm/kernel/process.c
> +++ b/arch/arm/kernel/process.c
> @@ -322,11 +322,6 @@ unsigned long get_wchan(struct task_struct *p)
> return 0;
> }
>
> -unsigned long arch_randomize_brk(struct mm_struct *mm)
> -{
> - return randomize_page(mm->brk, 0x02000000);
> -}
> -
> #ifdef CONFIG_MMU
> #ifdef CONFIG_KUSER_HELPERS
> /*
> 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-05-29 20:12:38

by Kees Cook

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

On Sun, May 26, 2019 at 09:47:36AM -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]>
> Reviewed-by: Christoph Hellwig <[email protected]>

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

-Kees

> ---
> arch/Kconfig | 10 ++++
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/processor.h | 2 -
> arch/arm64/mm/mmap.c | 76 -----------------------------
> kernel/sysctl.c | 6 ++-
> mm/util.c | 78 +++++++++++++++++++++++++++++-
> 6 files changed, 92 insertions(+), 81 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index c47b328eada0..df3ab04270fa 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -701,6 +701,16 @@ 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).
> +# Architecture that selects this option can provide its own version of:
> +# - STACK_RND_MASK
> +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 4780eb7af842..3d754c19c11e 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -69,6 +69,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 fcd0e691b1ea..3bd818edf319 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -282,8 +282,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 943c89178e3d..aebd03cc4b65 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -271,7 +271,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
>
> @@ -1566,7 +1567,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 dab33b896146..717f5d75c16e 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,78 @@ 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_HAVE_ARCH_MMAP_RND_COMPAT_BITS
> + if (is_compat_task())
> + rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> + else
> +#endif /* CONFIG_HAVE_ARCH_MMAP_RND_COMPAT_BITS */
> + 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;
> +}
> +
> +/*
> + * 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 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-05-29 20:13:05

by Kees Cook

Subject: Re: [PATCH v4 05/14] arm64, mm: Make randomization selected by generic topdown mmap layout

On Sun, May 26, 2019 at 09:47:37AM -0400, Alexandre Ghiti wrote:
> This commits selects ARCH_HAS_ELF_RANDOMIZE when an arch uses the generic
> topdown mmap layout functions so that this security feature is on by
> default.
> Note that this commit also removes the possibility for arm64 to have elf
> randomization and no MMU: without MMU, the security added by randomization
> is worth nothing.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

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

-Kees

> ---
> arch/Kconfig | 1 +
> arch/arm64/Kconfig | 1 -
> arch/arm64/kernel/process.c | 8 --------
> mm/util.c | 11 +++++++++--
> 4 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index df3ab04270fa..3732654446cc 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -710,6 +710,7 @@ config HAVE_ARCH_COMPAT_MMAP_BASES
> config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> bool
> depends on MMU
> + select ARCH_HAS_ELF_RANDOMIZE
>
> config HAVE_COPY_THREAD_TLS
> bool
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 3d754c19c11e..403bd3fffdbc 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -15,7 +15,6 @@ config ARM64
> select ARCH_HAS_DMA_MMAP_PGPROT
> select ARCH_HAS_DMA_PREP_COHERENT
> select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
> - select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_FAST_MULTIPLIER
> select ARCH_HAS_FORTIFY_SOURCE
> select ARCH_HAS_GCOV_PROFILE_ALL
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb21a5b8..3f85f8f2d665 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -535,14 +535,6 @@ unsigned long arch_align_stack(unsigned long sp)
> return sp & ~0xf;
> }
>
> -unsigned long arch_randomize_brk(struct mm_struct *mm)
> -{
> - if (is_compat_task())
> - return randomize_page(mm->brk, SZ_32M);
> - else
> - return randomize_page(mm->brk, SZ_1G);
> -}
> -
> /*
> * Called from setup_new_exec() after (COMPAT_)SET_PERSONALITY.
> */
> diff --git a/mm/util.c b/mm/util.c
> index 717f5d75c16e..8a38126edc74 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -319,7 +319,15 @@ unsigned long randomize_stack_top(unsigned long stack_top)
> }
>
> #ifdef CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> -#ifdef CONFIG_ARCH_HAS_ELF_RANDOMIZE
> +unsigned long arch_randomize_brk(struct mm_struct *mm)
> +{
> + /* Is the current task 32bit ? */
> + if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task())
> + return randomize_page(mm->brk, SZ_32M);
> +
> + return randomize_page(mm->brk, SZ_1G);
> +}
> +
> unsigned long arch_mmap_rnd(void)
> {
> unsigned long rnd;
> @@ -333,7 +341,6 @@ unsigned long arch_mmap_rnd(void)
>
> return rnd << PAGE_SHIFT;
> }
> -#endif /* CONFIG_ARCH_HAS_ELF_RANDOMIZE */
>
> static int mmap_is_legacy(struct rlimit *rlim_stack)
> {
> --
> 2.20.1
>

--
Kees Cook

2019-05-29 20:14:20

by Kees Cook

Subject: Re: [PATCH v4 12/14] mips: Replace arch specific way to determine 32bit task with generic version

On Sun, May 26, 2019 at 09:47:44AM -0400, Alexandre Ghiti wrote:
> Mips uses TASK_IS_32BIT_ADDR to determine if a task is 32bit, but
> this define is mips specific and other arches do not have it: instead,
> use !IS_ENABLED(CONFIG_64BIT) || is_compat_task() condition.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

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

-Kees

> ---
> arch/mips/mm/mmap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index c052565b76fb..900670ea8531 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -17,6 +17,7 @@
> #include <linux/sched/signal.h>
> #include <linux/sched/mm.h>
> #include <linux/sizes.h>
> +#include <linux/compat.h>
>
> unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
> EXPORT_SYMBOL(shm_align_mask);
> @@ -191,7 +192,7 @@ static inline unsigned long brk_rnd(void)
>
> rnd = rnd << PAGE_SHIFT;
> /* 32MB for 32bit, 1GB for 64bit */
> - if (TASK_IS_32BIT_ADDR)
> + if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task())
> rnd = rnd & SZ_32M;
> else
> rnd = rnd & SZ_1G;
> --
> 2.20.1
>

--
Kees Cook

2019-05-29 20:15:17

by Kees Cook

Subject: Re: [PATCH v4 13/14] mips: Use generic mmap top-down layout and brk randomization

On Sun, May 26, 2019 at 09:47:45AM -0400, Alexandre Ghiti wrote:
> mips uses a top-down 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.
> As ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT selects ARCH_HAS_ELF_RANDOMIZE,
> use the generic version of arch_randomize_brk since it also fits.
> Note that this commit also removes the possibility for mips to have elf
> randomization and no MMU: without MMU, the security added by randomization
> is worth nothing.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

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

-Kees

> ---
> arch/mips/Kconfig | 2 +-
> arch/mips/include/asm/processor.h | 5 --
> arch/mips/mm/mmap.c | 96 -------------------------------
> 3 files changed, 1 insertion(+), 102 deletions(-)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 70d3200476bf..da15b02bbe23 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -5,7 +5,6 @@ config MIPS
> select ARCH_32BIT_OFF_T if !64BIT
> select ARCH_BINFMT_ELF_STATE if MIPS_FP_SUPPORT
> select ARCH_CLOCKSOURCE_DATA
> - select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> select ARCH_SUPPORTS_UPROBES
> @@ -13,6 +12,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 900670ea8531..c2effe535484 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -16,49 +16,10 @@
> #include <linux/random.h>
> #include <linux/sched/signal.h>
> #include <linux/sched/mm.h>
> -#include <linux/sizes.h>
> -#include <linux/compat.h>
>
> 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))
> @@ -156,63 +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();
> -
> - rnd = rnd << PAGE_SHIFT;
> - /* 32MB for 32bit, 1GB for 64bit */
> - if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task())
> - rnd = rnd & SZ_32M;
> - else
> - rnd = rnd & SZ_1G;
> -
> - return rnd;
> -}
> -
> -unsigned long arch_randomize_brk(struct mm_struct *mm)
> -{
> - unsigned long base = mm->brk;
> - unsigned long ret;
> -
> - ret = PAGE_ALIGN(base + brk_rnd());
> -
> - if (ret < mm->brk)
> - return mm->brk;
> -
> - return ret;
> -}
> -
> int __virt_addr_valid(const volatile void *kaddr)
> {
> return pfn_valid(PFN_DOWN(virt_to_phys(kaddr)));
> --
> 2.20.1
>

--
Kees Cook

2019-05-29 20:15:45

by Kees Cook

Subject: Re: [PATCH v4 11/14] mips: Adjust brk randomization offset to fit generic version

On Sun, May 26, 2019 at 09:47:43AM -0400, Alexandre Ghiti wrote:
> This commit simply bumps up to 32MB and 1GB the random offset
> of brk, compared to 8MB and 256MB, for 32bit and 64bit respectively.
>
> Suggested-by: Kees Cook <[email protected]>
> Signed-off-by: Alexandre Ghiti <[email protected]>

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

-Kees

> ---
> arch/mips/mm/mmap.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index ffbe69f3a7d9..c052565b76fb 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -16,6 +16,7 @@
> #include <linux/random.h>
> #include <linux/sched/signal.h>
> #include <linux/sched/mm.h>
> +#include <linux/sizes.h>
>
> unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */
> EXPORT_SYMBOL(shm_align_mask);
> @@ -189,11 +190,11 @@ static inline unsigned long brk_rnd(void)
> unsigned long rnd = get_random_long();
>
> rnd = rnd << PAGE_SHIFT;
> - /* 8MB for 32bit, 256MB for 64bit */
> + /* 32MB for 32bit, 1GB for 64bit */
> if (TASK_IS_32BIT_ADDR)
> - rnd = rnd & 0x7ffffful;
> + rnd = rnd & SZ_32M;
> else
> - rnd = rnd & 0xffffffful;
> + rnd = rnd & SZ_1G;
>
> return rnd;
> }
> --
> 2.20.1
>

--
Kees Cook

2019-05-29 20:18:24

by Kees Cook

Subject: Re: [PATCH v4 00/14] Provide generic top-down mmap layout functions

On Sun, May 26, 2019 at 09:47:32AM -0400, Alexandre Ghiti wrote:
> 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.

As I've mentioned before, I think this is really great. Making this
common has long been on my TODO list. Thank you for the work! (I've sent
separate review emails for individual patches where my ack wasn't
already present...)

> - There is no common API to determine if a process is 32b, so I came up with
> !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14].

Do we need a common helper for this idiom? (Note that I don't think it's
worth blocking the series for this.)

-Kees

--
Kees Cook

2019-05-31 04:51:16

by Alexandre Ghiti

Subject: Re: [PATCH v4 08/14] arm: Use generic mmap top-down layout and brk randomization

On 5/29/19 3:26 PM, Kees Cook wrote:
> On Sun, May 26, 2019 at 09:47:40AM -0400, Alexandre Ghiti 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.
>> As ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT selects ARCH_HAS_ELF_RANDOMIZE,
>> use the generic version of arch_randomize_brk since it also fits.
>> Note that this commit also removes the possibility for arm to have elf
>> randomization and no MMU: without MMU, the security added by randomization
>> is worth nothing.
>>
>> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>
>
> It may be worth noting that STACK_RND_MASK is safe to remove here
> because it matches the default that now exists in mm/util.c.


Yes, thanks for pointing that.


Thanks,


Alex


>
> -Kees
>
>> ---
>> arch/arm/Kconfig | 2 +-
>> arch/arm/include/asm/processor.h | 2 --
>> arch/arm/kernel/process.c | 5 ---
>> arch/arm/mm/mmap.c | 62 --------------------------------
>> 4 files changed, 1 insertion(+), 70 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 8869742a85df..27687a8c9fb5 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -6,7 +6,6 @@ config ARM
>> select ARCH_CLOCKSOURCE_DATA
>> select ARCH_HAS_DEBUG_VIRTUAL if MMU
>> select ARCH_HAS_DEVMEM_IS_ALLOWED
>> - select ARCH_HAS_ELF_RANDOMIZE
>> select ARCH_HAS_FORTIFY_SOURCE
>> select ARCH_HAS_KEEPINITRD
>> select ARCH_HAS_KCOV
>> @@ -29,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 5d06f75ffad4..95b7688341c5 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/kernel/process.c b/arch/arm/kernel/process.c
>> index 72cc0862a30e..19a765db5f7f 100644
>> --- a/arch/arm/kernel/process.c
>> +++ b/arch/arm/kernel/process.c
>> @@ -322,11 +322,6 @@ unsigned long get_wchan(struct task_struct *p)
>> return 0;
>> }
>>
>> -unsigned long arch_randomize_brk(struct mm_struct *mm)
>> -{
>> - return randomize_page(mm->brk, 0x02000000);
>> -}
>> -
>> #ifdef CONFIG_MMU
>> #ifdef CONFIG_KUSER_HELPERS
>> /*
>> 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-05-31 05:06:44

by Alexandre Ghiti

Subject: Re: [PATCH v4 00/14] Provide generic top-down mmap layout functions

On 5/29/19 4:16 PM, Kees Cook wrote:
> On Sun, May 26, 2019 at 09:47:32AM -0400, Alexandre Ghiti wrote:
>> 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.
> As I've mentioned before, I think this is really great. Making this
> common has long been on my TODO list. Thank you for the work! (I've sent
> separate review emails for individual patches where my ack wasn't
> already present...)


Thanks :)


>> - There is no common API to determine if a process is 32b, so I came up with
>> !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14].
> Do we need a common helper for this idiom? (Note that I don't think it's
> worth blocking the series for this.)


Each architecture has its own way of finding that out, it might be
interesting if there are other
places in generic code to propose something in that sense.
I will search for such places if they exist and come back with something.

Thanks Kees for your time,

Alex


>
> -Kees
>

2019-06-01 09:06:27

by Christoph Hellwig

Subject: Re: [PATCH v4 05/14] arm64, mm: Make randomization selected by generic topdown mmap layout

Looks good,

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

2019-06-03 07:27:35

by Alexandre Ghiti

Subject: Re: [PATCH v4 05/14] arm64, mm: Make randomization selected by generic topdown mmap layout

On 6/1/19 11:04 AM, Christoph Hellwig wrote:
> Looks good,
>
> Reviewed-by: Christoph Hellwig <[email protected]>


Thanks for your time,

Alex


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

2019-06-03 17:56:17

by Catalin Marinas

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

On Sun, May 26, 2019 at 09:47:34AM -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]>
> Acked-by: Kees Cook <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>

Acked-by: Catalin Marinas <[email protected]>

2019-06-03 17:56:33

by Catalin Marinas

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

On Sun, May 26, 2019 at 09:47:35AM -0400, Alexandre Ghiti wrote:
> Do not offset mmap base address because of stack randomization if
> current task does not want randomization.
> Note that x86 already implements this behaviour.
>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> Acked-by: Kees Cook <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>

Acked-by: Catalin Marinas <[email protected]>

2019-06-03 17:56:38

by Catalin Marinas

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

On Sun, May 26, 2019 at 09:47:36AM -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]>
> Reviewed-by: Christoph Hellwig <[email protected]>

Acked-by: Catalin Marinas <[email protected]>

2019-06-03 17:56:45

by Catalin Marinas

Subject: Re: [PATCH v4 05/14] arm64, mm: Make randomization selected by generic topdown mmap layout

On Sun, May 26, 2019 at 09:47:37AM -0400, Alexandre Ghiti wrote:
> This commits selects ARCH_HAS_ELF_RANDOMIZE when an arch uses the generic
> topdown mmap layout functions so that this security feature is on by
> default.
> Note that this commit also removes the possibility for arm64 to have elf
> randomization and no MMU: without MMU, the security added by randomization
> is worth nothing.

Not planning on this anytime soon ;).

Acked-by: Catalin Marinas <[email protected]>

2019-06-04 06:22:11

by Alexandre Ghiti

Subject: Re: [PATCH v4 05/14] arm64, mm: Make randomization selected by generic topdown mmap layout

On 6/3/19 1:40 PM, Catalin Marinas wrote:
> On Sun, May 26, 2019 at 09:47:37AM -0400, Alexandre Ghiti wrote:
>> This commits selects ARCH_HAS_ELF_RANDOMIZE when an arch uses the generic
>> topdown mmap layout functions so that this security feature is on by
>> default.
>> Note that this commit also removes the possibility for arm64 to have elf
>> randomization and no MMU: without MMU, the security added by randomization
>> is worth nothing.
> Not planning on this anytime soon ;).


Great :) Thanks for your time,

Alex


>
> Acked-by: Catalin Marinas <[email protected]>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2019-06-13 16:46:59

by Alexandre Ghiti

Subject: Re: [PATCH v4 00/14] Provide generic top-down mmap layout functions

On 5/26/19 9:47 AM, Alexandre Ghiti wrote:
> 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 v4:
> - Make ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select ARCH_HAS_ELF_RANDOMIZE
> by default as suggested by Kees,
> - ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT depends on MMU and defines the
> functions needed by ARCH_HAS_ELF_RANDOMIZE => architectures that use
> the generic mmap topdown functions cannot have ARCH_HAS_ELF_RANDOMIZE
> selected without MMU, but I think it's ok since randomization without
> MMU does not add much security anyway.
> - There is no common API to determine if a process is 32b, so I came up with
> !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14].
> - Mention in the change log that x86 already takes care of not offseting mmap
> base address if the task does not want randomization.
> - Re-introduce a comment that should not have been removed.
> - Add Reviewed/Acked-By from Paul, Christoph and Kees, thank you for that.
> - I tried to minimize the changes from the commits in v3 in order to make
> easier the review of the v4, the commits changed or added are:
> - [PATCH v4 5/14]
> - [PATCH v4 8/14]
> - [PATCH v4 11/14]
> - [PATCH v4 12/14]
> - [PATCH v4 13/14]

Hi Paul,

Compared to the previous version you already acked, patches 11, 12 and 13
would need your feedback, do you have time to take a look at them ?

Hope I don't bother you,

Thanks,

Alex


>
> 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 (14):
> 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
> arm64, mm: Make randomization selected by generic topdown mmap layout
> 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 and brk randomization
> mips: Properly account for stack randomization and stack guard gap
> mips: Use STACK_TOP when computing mmap base address
> mips: Adjust brk randomization offset to fit generic version
> mips: Replace arch specific way to determine 32bit task with generic
> version
> mips: Use generic mmap top-down layout and brk randomization
> riscv: Make mmap allocation top-down by default
>
> arch/Kconfig | 11 +++
> arch/arm/Kconfig | 2 +-
> arch/arm/include/asm/processor.h | 2 -
> arch/arm/kernel/process.c | 5 --
> arch/arm/mm/mmap.c | 52 --------------
> arch/arm64/Kconfig | 2 +-
> arch/arm64/include/asm/processor.h | 2 -
> arch/arm64/kernel/process.c | 8 ---
> arch/arm64/mm/mmap.c | 72 -------------------
> arch/mips/Kconfig | 2 +-
> arch/mips/include/asm/processor.h | 5 --
> arch/mips/mm/mmap.c | 84 ----------------------
> arch/riscv/Kconfig | 11 +++
> fs/binfmt_elf.c | 20 ------
> include/linux/mm.h | 2 +
> kernel/sysctl.c | 6 +-
> mm/util.c | 107 ++++++++++++++++++++++++++++-
> 17 files changed, 137 insertions(+), 256 deletions(-)
>

2019-06-30 15:35:20

by Alexandre Ghiti

Subject: Re: [PATCH v4 00/14] Provide generic top-down mmap layout functions

On 6/13/19 1:29 AM, Alex Ghiti wrote:
> On 5/26/19 9:47 AM, Alexandre Ghiti wrote:
>> 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 v4:
>> ?? - Make ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select
>> ARCH_HAS_ELF_RANDOMIZE
>> ???? by default as suggested by Kees,
>> ?? - ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT depends on MMU and defines
>> the
>> ???? functions needed by ARCH_HAS_ELF_RANDOMIZE => architectures that
>> use
>> ???? the generic mmap topdown functions cannot have
>> ARCH_HAS_ELF_RANDOMIZE
>> ???? selected without MMU, but I think it's ok since randomization
>> without
>> ???? MMU does not add much security anyway.
>> ?? - There is no common API to determine if a process is 32b, so I
>> came up with
>> ???? !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14].
>> ?? - Mention in the change log that x86 already takes care of not
>> offseting mmap
>> ???? base address if the task does not want randomization.
>> ?? - Re-introduce a comment that should not have been removed.
>> ?? - Add Reviewed/Acked-By from Paul, Christoph and Kees, thank you
>> for that.
>> ?? - I tried to minimize the changes from the commits in v3 in order
>> to make
>> ???? easier the review of the v4, the commits changed or added are:
>> ???? - [PATCH v4 5/14]
>> ???? - [PATCH v4 8/14]
>> ???? - [PATCH v4 11/14]
>> ???? - [PATCH v4 12/14]
>> ???? - [PATCH v4 13/14]
>
> Hi Paul,
>
> Compared to the previous version you already acked, patches 11, 12 and 13
> would need your feedback, do you have time to take a look at them ?
>
> Hope I don't bother you,
>
> Thanks,
>
> Alex
>

Hi Paul,

Would you have time to give your feedback on patches 11, 12 and 13 ?

Thanks,

Alex


>
>>
>> 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 (14):
>> ?? 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
>> ?? arm64, mm: Make randomization selected by generic topdown mmap layout
>> ?? 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 and brk randomization
>> ?? mips: Properly account for stack randomization and stack guard gap
>> ?? mips: Use STACK_TOP when computing mmap base address
>> ?? mips: Adjust brk randomization offset to fit generic version
>> ?? mips: Replace arch specific way to determine 32bit task with generic
>> ???? version
>> ?? mips: Use generic mmap top-down layout and brk randomization
>> ?? riscv: Make mmap allocation top-down by default
>>
>> ? arch/Kconfig?????????????????????? |? 11 +++
>> ? arch/arm/Kconfig?????????????????? |?? 2 +-
>> ? arch/arm/include/asm/processor.h?? |?? 2 -
>> ? arch/arm/kernel/process.c????????? |?? 5 --
>> ? arch/arm/mm/mmap.c???????????????? |? 52 --------------
>> ? arch/arm64/Kconfig???????????????? |?? 2 +-
>> ? arch/arm64/include/asm/processor.h |?? 2 -
>> ? arch/arm64/kernel/process.c??????? |?? 8 ---
>> ? arch/arm64/mm/mmap.c?????????????? |? 72 -------------------
>> ? arch/mips/Kconfig????????????????? |?? 2 +-
>> ? arch/mips/include/asm/processor.h? |?? 5 --
>> ? arch/mips/mm/mmap.c??????????????? |? 84 ----------------------
>> ? arch/riscv/Kconfig???????????????? |? 11 +++
>> ? fs/binfmt_elf.c??????????????????? |? 20 ------
>> ? include/linux/mm.h???????????????? |?? 2 +
>> ? kernel/sysctl.c??????????????????? |?? 6 +-
>> ? mm/util.c????????????????????????? | 107 ++++++++++++++++++++++++++++-
>> ? 17 files changed, 137 insertions(+), 256 deletions(-)
>>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv