Christophe Leroy [off-list ref] writes:

Le 09/12/2021 à 12:22, Michael Ellerman a écrit :

quoted

Nicholas Piggin [off-list ref] writes:

quoted

Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm:

quoted

Le 09/12/2021 à 11:15, Nicholas Piggin a écrit :

quoted

Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am:

quoted

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.

quoted

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