| Message ID | d2d5510115cba2d56866fa01dab267655a20da71.1638976229.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert powerpc to default topdown mmap layout | expand |
Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: > Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and > remove arch/powerpc/mm/mmap.c > > This change provides standard randomisation of mmaps. > > See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 > and X86_32") for all the benefits of mmap randomisation. The justification seems pretty reasonable. > > Comparison between powerpc implementation and the generic one: > - mmap_is_legacy() is identical. > - arch_mmap_rnd() does exactly the same allthough it's written > slightly differently. > - MIN_GAP and MAX_GAP are identical. > - mmap_base() does the same but uses STACK_RND_MASK which provides > the same values as stack_maxrandom_size(). > - arch_pick_mmap_layout() is almost identical. The only difference > is that it also adds the random factor to mm->mmap_base in legacy mode. > > That last point is what provides the standard randomisation of mmaps. Thanks for describing it. Could you add random_factor to mmap_base for the legacy path for powerpc as a 2-line change that adds the legacy randomisation. And then this bigger patch would be closer to a no-op. Thanks, Nick
Le 09/12/2021 à 11:15, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and >> remove arch/powerpc/mm/mmap.c >> >> This change provides standard randomisation of mmaps. >> >> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 >> and X86_32") for all the benefits of mmap randomisation. > > The justification seems pretty reasonable. > >> >> Comparison between powerpc implementation and the generic one: >> - mmap_is_legacy() is identical. >> - arch_mmap_rnd() does exactly the same allthough it's written >> slightly differently. >> - MIN_GAP and MAX_GAP are identical. >> - mmap_base() does the same but uses STACK_RND_MASK which provides >> the same values as stack_maxrandom_size(). >> - arch_pick_mmap_layout() is almost identical. The only difference >> is that it also adds the random factor to mm->mmap_base in legacy mode. >> >> That last point is what provides the standard randomisation of mmaps. > > Thanks for describing it. Could you add random_factor to mmap_base for > the legacy path for powerpc as a 2-line change that adds the legacy > randomisation. And then this bigger patch would be closer to a no-op. > You mean you would like to see the following patch before doing the convert ? https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7dabf1cbde67a346a187881d4f0bd17347e0334a.1533732583.git.christophe.leroy@c-s.fr/
Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm: > > > Le 09/12/2021 à 11:15, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >>> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and >>> remove arch/powerpc/mm/mmap.c >>> >>> This change provides standard randomisation of mmaps. >>> >>> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 >>> and X86_32") for all the benefits of mmap randomisation. >> >> The justification seems pretty reasonable. >> >>> >>> Comparison between powerpc implementation and the generic one: >>> - mmap_is_legacy() is identical. >>> - arch_mmap_rnd() does exactly the same allthough it's written >>> slightly differently. >>> - MIN_GAP and MAX_GAP are identical. >>> - mmap_base() does the same but uses STACK_RND_MASK which provides >>> the same values as stack_maxrandom_size(). >>> - arch_pick_mmap_layout() is almost identical. The only difference >>> is that it also adds the random factor to mm->mmap_base in legacy mode. >>> >>> That last point is what provides the standard randomisation of mmaps. >> >> Thanks for describing it. Could you add random_factor to mmap_base for >> the legacy path for powerpc as a 2-line change that adds the legacy >> randomisation. And then this bigger patch would be closer to a no-op. >> > > You mean you would like to see the following patch before doing the > convert ? > > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7dabf1cbde67a346a187881d4f0bd17347e0334a.1533732583.git.christophe.leroy@c-s.fr/ Yes. Thanks, Nick
Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm: >> >> >> Le 09/12/2021 à 11:15, Nicholas Piggin a écrit : >>> Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >>>> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and >>>> remove arch/powerpc/mm/mmap.c >>>> >>>> This change provides standard randomisation of mmaps. >>>> >>>> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 >>>> and X86_32") for all the benefits of mmap randomisation. >>> >>> The justification seems pretty reasonable. >>> >>>> >>>> Comparison between powerpc implementation and the generic one: >>>> - mmap_is_legacy() is identical. >>>> - arch_mmap_rnd() does exactly the same allthough it's written >>>> slightly differently. >>>> - MIN_GAP and MAX_GAP are identical. >>>> - mmap_base() does the same but uses STACK_RND_MASK which provides >>>> the same values as stack_maxrandom_size(). >>>> - arch_pick_mmap_layout() is almost identical. The only difference >>>> is that it also adds the random factor to mm->mmap_base in legacy mode. >>>> >>>> That last point is what provides the standard randomisation of mmaps. >>> >>> Thanks for describing it. Could you add random_factor to mmap_base for >>> the legacy path for powerpc as a 2-line change that adds the legacy >>> randomisation. And then this bigger patch would be closer to a no-op. >>> >> >> You mean you would like to see the following patch before doing the >> convert ? >> >> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7dabf1cbde67a346a187881d4f0bd17347e0334a.1533732583.git.christophe.leroy@c-s.fr/ > > Yes. My comment at the time was: Basically mmap_is_legacy() tells you if any of these is true: - process has the ADDR_COMPAT_LAYOUT personality - global legacy_va_layout sysctl is enabled - stack is unlimited And we only want to change the behaviour for the stack. Or at least the change log of your patch only talks about the stack limit, not the others. Possibly we should just enable randomisation for all three of those cases, but if so we must spell it out in the patch. It'd also be good to see the output of /proc/x/maps for some processes before and after, to show what actually changes. From: https://github.com/linuxppc/issues/issues/59#issuecomment-502066947 So I think at least the change log on that patch still needs updating to be clear that it's changing behaviour for all mmap_is_legacy() cases, not just the stack unlimited case. There's also a risk changing the mmap legacy behaviour breaks something. But we are at least matching the behaviour of other architectures, and there is also an escape hatch in the form of `setarch -R`. cheers
Le 09/12/2021 à 12:22, Michael Ellerman a écrit : > Nicholas Piggin <npiggin@gmail.com> writes: > >> Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm: >>> >>> >>> Le 09/12/2021 à 11:15, Nicholas Piggin a écrit : >>>> Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >>>>> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and >>>>> remove arch/powerpc/mm/mmap.c >>>>> >>>>> This change provides standard randomisation of mmaps. >>>>> >>>>> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 >>>>> and X86_32") for all the benefits of mmap randomisation. >>>> >>>> The justification seems pretty reasonable. >>>> >>>>> >>>>> Comparison between powerpc implementation and the generic one: >>>>> - mmap_is_legacy() is identical. >>>>> - arch_mmap_rnd() does exactly the same allthough it's written >>>>> slightly differently. >>>>> - MIN_GAP and MAX_GAP are identical. >>>>> - mmap_base() does the same but uses STACK_RND_MASK which provides >>>>> the same values as stack_maxrandom_size(). >>>>> - arch_pick_mmap_layout() is almost identical. The only difference >>>>> is that it also adds the random factor to mm->mmap_base in legacy mode. >>>>> >>>>> That last point is what provides the standard randomisation of mmaps. >>>> >>>> Thanks for describing it. Could you add random_factor to mmap_base for >>>> the legacy path for powerpc as a 2-line change that adds the legacy >>>> randomisation. And then this bigger patch would be closer to a no-op. >>>> >>> >>> You mean you would like to see the following patch before doing the >>> convert ? >>> >>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7dabf1cbde67a346a187881d4f0bd17347e0334a.1533732583.git.christophe.leroy@c-s.fr/ >> >> Yes. > > My comment at the time was: > > Basically mmap_is_legacy() tells you if any of these is true: > > - process has the ADDR_COMPAT_LAYOUT personality > - global legacy_va_layout sysctl is enabled > - stack is unlimited > > And we only want to change the behaviour for the stack. Or at least the > change log of your patch only talks about the stack limit, not the > others. > > Possibly we should just enable randomisation for all three of those > cases, but if so we must spell it out in the patch. > > It'd also be good to see the output of /proc/x/maps for some processes > before and after, to show what actually changes. > > > From: https://github.com/linuxppc/issues/issues/59#issuecomment-502066947 > > > So I think at least the change log on that patch still needs updating to > be clear that it's changing behaviour for all mmap_is_legacy() cases, > not just the stack unlimited case. > > There's also a risk changing the mmap legacy behaviour breaks something. > But we are at least matching the behaviour of other architectures, and > there is also an escape hatch in the form of `setarch -R`. > That was the purpose of adding in the change log a reference to commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 and X86_32") All this applies to powerpc as well. But I can copy paste the changelog of that commit into mine if you think it is more explicit. I agree that old patch was only refering to stack limit, I had no clue of everything else at that time. Christophe
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 09/12/2021 à 12:22, Michael Ellerman a écrit : >> Nicholas Piggin <npiggin@gmail.com> writes: >> >>> Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm: >>>> >>>> >>>> Le 09/12/2021 à 11:15, Nicholas Piggin a écrit : >>>>> Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >>>>>> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and >>>>>> remove arch/powerpc/mm/mmap.c >>>>>> >>>>>> This change provides standard randomisation of mmaps. >>>>>> >>>>>> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 >>>>>> and X86_32") for all the benefits of mmap randomisation. >>>>> >>>>> The justification seems pretty reasonable. >>>>> >>>>>> >>>>>> Comparison between powerpc implementation and the generic one: >>>>>> - mmap_is_legacy() is identical. >>>>>> - arch_mmap_rnd() does exactly the same allthough it's written >>>>>> slightly differently. >>>>>> - MIN_GAP and MAX_GAP are identical. >>>>>> - mmap_base() does the same but uses STACK_RND_MASK which provides >>>>>> the same values as stack_maxrandom_size(). >>>>>> - arch_pick_mmap_layout() is almost identical. The only difference >>>>>> is that it also adds the random factor to mm->mmap_base in legacy mode. >>>>>> >>>>>> That last point is what provides the standard randomisation of mmaps. >>>>> >>>>> Thanks for describing it. Could you add random_factor to mmap_base for >>>>> the legacy path for powerpc as a 2-line change that adds the legacy >>>>> randomisation. And then this bigger patch would be closer to a no-op. >>>>> >>>> >>>> You mean you would like to see the following patch before doing the >>>> convert ? >>>> >>>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7dabf1cbde67a346a187881d4f0bd17347e0334a.1533732583.git.christophe.leroy@c-s.fr/ >>> >>> Yes. >> >> My comment at the time was: >> >> Basically mmap_is_legacy() tells you if any of these is true: >> >> - process has the ADDR_COMPAT_LAYOUT personality >> - global legacy_va_layout sysctl is enabled >> - stack is unlimited >> >> And we only want to change the behaviour for the stack. Or at least the >> change log of your patch only talks about the stack limit, not the >> others. >> >> Possibly we should just enable randomisation for all three of those >> cases, but if so we must spell it out in the patch. >> >> It'd also be good to see the output of /proc/x/maps for some processes >> before and after, to show what actually changes. >> >> >> From: https://github.com/linuxppc/issues/issues/59#issuecomment-502066947 >> >> >> So I think at least the change log on that patch still needs updating to >> be clear that it's changing behaviour for all mmap_is_legacy() cases, >> not just the stack unlimited case. >> >> There's also a risk changing the mmap legacy behaviour breaks something. >> But we are at least matching the behaviour of other architectures, and >> there is also an escape hatch in the form of `setarch -R`. > > That was the purpose of adding in the change log a reference to commit > 8b8addf891de ("x86/mm/32: Enable full randomization on i386 > and X86_32") > > All this applies to powerpc as well. Yeah, I'm just a pessimist :) So although the security benefit is nice, I'm more worried that the layout change will break some mission critical legacy app somewhere. So I just like to have that spelled out in the change log, or at least in the discussion like here. > But I can copy paste the changelog of that commit into mine if you think > it is more explicit. Just referring to it is probably fine. > I agree that old patch was only refering to stack limit, I had no clue > of everything else at that time. No worries. cheers
Le 09/12/2021 à 11:15, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am: >> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and >> remove arch/powerpc/mm/mmap.c >> >> This change provides standard randomisation of mmaps. >> >> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 >> and X86_32") for all the benefits of mmap randomisation. > > The justification seems pretty reasonable. > >> >> Comparison between powerpc implementation and the generic one: >> - mmap_is_legacy() is identical. >> - arch_mmap_rnd() does exactly the same allthough it's written >> slightly differently. >> - MIN_GAP and MAX_GAP are identical. >> - mmap_base() does the same but uses STACK_RND_MASK which provides >> the same values as stack_maxrandom_size(). >> - arch_pick_mmap_layout() is almost identical. The only difference >> is that it also adds the random factor to mm->mmap_base in legacy mode. >> >> That last point is what provides the standard randomisation of mmaps. > > Thanks for describing it. Could you add random_factor to mmap_base for > the legacy path for powerpc as a 2-line change that adds the legacy > randomisation. And then this bigger patch would be closer to a no-op. > Ok, in v5 I added that change in patch 10 then switched this patch with that patch. Christophe
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 0631c9241af3..b4ae3d8bde46 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -122,7 +122,6 @@ config PPC select ARCH_HAS_DEBUG_WX if STRICT_KERNEL_RWX select ARCH_HAS_DEVMEM_IS_ALLOWED select ARCH_HAS_DMA_MAP_DIRECT if PPC_PSERIES - select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_HUGEPD if HUGETLB_PAGE @@ -158,6 +157,7 @@ config PPC select ARCH_USE_MEMTEST select ARCH_USE_QUEUED_RWLOCKS if PPC_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_SPINLOCKS if PPC_QUEUED_SPINLOCKS + select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select ARCH_WANT_IPC_PARSE_VERSION select ARCH_WANT_IRQS_OFF_ACTIVATE_MM select ARCH_WANT_LD_ORPHAN_WARN diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h index 2c8686d9e964..873adaab20c8 100644 --- a/arch/powerpc/include/asm/processor.h +++ b/arch/powerpc/include/asm/processor.h @@ -392,8 +392,6 @@ static inline void prefetchw(const void *x) #define spin_lock_prefetch(x) prefetchw(x) -#define HAVE_ARCH_PICK_MMAP_LAYOUT - /* asm stubs */ extern unsigned long isa300_idle_stop_noloss(unsigned long psscr_val); extern unsigned long isa300_idle_stop_mayloss(unsigned long psscr_val); diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile index d4c20484dad9..503a6e249940 100644 --- a/arch/powerpc/mm/Makefile +++ b/arch/powerpc/mm/Makefile @@ -5,7 +5,7 @@ ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC) -obj-y := fault.o mem.o pgtable.o mmap.o maccess.o pageattr.o \ +obj-y := fault.o mem.o pgtable.o maccess.o pageattr.o \ init_$(BITS).o pgtable_$(BITS).o \ pgtable-frag.o ioremap.o ioremap_$(BITS).o \ init-common.o mmu_context.o drmem.o \ diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c deleted file mode 100644 index 5972d619d274..000000000000 --- a/arch/powerpc/mm/mmap.c +++ /dev/null @@ -1,105 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -/* - * flexible mmap layout support - * - * Copyright 2003-2004 Red Hat Inc., Durham, North Carolina. - * All Rights Reserved. - * - * Started by Ingo Molnar <mingo@elte.hu> - */ - -#include <linux/personality.h> -#include <linux/mm.h> -#include <linux/random.h> -#include <linux/sched/signal.h> -#include <linux/sched/mm.h> -#include <linux/elf-randomize.h> -#include <linux/security.h> -#include <linux/mman.h> - -/* - * Top of mmap area (just below the process stack). - * - * Leave at least a ~128 MB hole. - */ -#define MIN_GAP (128*1024*1024) -#define MAX_GAP (TASK_SIZE/6*5) - -static inline 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 shift, rnd; - - shift = mmap_rnd_bits; -#ifdef CONFIG_COMPAT - if (is_32bit_task()) - shift = mmap_rnd_compat_bits; -#endif - rnd = get_random_long() % (1ul << shift); - - return rnd << PAGE_SHIFT; -} - -static inline unsigned long stack_maxrandom_size(void) -{ - if (!(current->flags & PF_RANDOMIZE)) - return 0; - - /* 8MB for 32bit, 1GB for 64bit */ - if (is_32bit_task()) - return (1<<23); - else - return (1<<30); -} - -static inline unsigned long mmap_base(unsigned long rnd, - struct rlimit *rlim_stack) -{ - unsigned long gap = rlim_stack->rlim_cur; - unsigned long pad = stack_maxrandom_size() + stack_guard_gap; - - /* 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(DEFAULT_MAP_WINDOW - 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; - 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; - } -}
Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and remove arch/powerpc/mm/mmap.c This change provides standard randomisation of mmaps. See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386 and X86_32") for all the benefits of mmap randomisation. Comparison between powerpc implementation and the generic one: - mmap_is_legacy() is identical. - arch_mmap_rnd() does exactly the same allthough it's written slightly differently. - MIN_GAP and MAX_GAP are identical. - mmap_base() does the same but uses STACK_RND_MASK which provides the same values as stack_maxrandom_size(). - arch_pick_mmap_layout() is almost identical. The only difference is that it also adds the random factor to mm->mmap_base in legacy mode. That last point is what provides the standard randomisation of mmaps. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/Kconfig | 2 +- arch/powerpc/include/asm/processor.h | 2 - arch/powerpc/mm/Makefile | 2 +- arch/powerpc/mm/mmap.c | 105 --------------------------- 4 files changed, 2 insertions(+), 109 deletions(-) delete mode 100644 arch/powerpc/mm/mmap.c