| Message ID | 20190417052247.17809-5-alex@ghiti.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Provide generic top-down mmap layout functions | expand |
( On Wed, Apr 17, 2019 at 12:27 AM Alexandre Ghiti <alex@ghiti.fr> 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 <hch@infradead.org> > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr> > --- > 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
On 4/18/19 1:17 AM, Kees Cook wrote: > ( > > On Wed, Apr 17, 2019 at 12:27 AM Alexandre Ghiti <alex@ghiti.fr> 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 <hch@infradead.org> >> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr> >> --- >> 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
On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <alex@ghiti.fr> 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. :)
On 4/18/19 10:19 AM, Kees Cook wrote: > On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <alex@ghiti.fr> 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
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 <hch@infradead.org> > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Apr 18, 2019 at 09:19:18AM -0500, Kees Cook wrote: > On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <alex@ghiti.fr> 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.
On 4/18/19 10:19 AM, Kees Cook wrote: > On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <alex@ghiti.fr> 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
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;
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 <hch@infradead.org> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr> --- 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(-)