Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
> Le 09/12/2021 à 12:22, Michael Ellerman a écrit :
>> Nicholas Piggin <npiggin@xxxxxxxxx> 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@xxxxxx/
>>>
>>> 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
