Re: [PATCH v4 09/10] powerpc/mm: Convert to default topdown mmap layout

[Michael Ellerman <mpe@ellerman.id.au>]

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