[lkml]   [2021]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v4 09/10] powerpc/mm: Convert to default topdown mmap layout
Christophe Leroy <> writes:
> Le 09/12/2021 à 12:22, Michael Ellerman a écrit :
>> Nicholas Piggin <> 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:
>>>>>> 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 ?
>>> 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:
>> 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.


 \ /
  Last update: 2021-12-10 00:56    [W:0.039 / U:4.676 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site