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 <[email protected]>
---
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
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 <[email protected]>
- */
-
-#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;
- }
-}
--
2.33.1
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> writes:
> Le 09/12/2021 à 12:22, Michael Ellerman a écrit :
>> Nicholas Piggin <[email protected]> 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