Re: [PATCH 1/2] powerpc: mm: support ARCH_MMAP_RND_BITS

[Bhupesh Sharma <bhsharma@redhat.com>]

HI Michael,

On Thu, Feb 2, 2017 at 3:53 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Bhupesh Sharma <bhsharma@redhat.com> writes:
>
>> powerpc: arch_mmap_rnd() uses hard-coded values, (23-PAGE_SHIFT) for
>> 32-bit and (30-PAGE_SHIFT) for 64-bit, to generate the random offset
>> for the mmap base address.
>>
>> This value represents a compromise between increased
>> ASLR effectiveness and avoiding address-space fragmentation.
>> Replace it with a Kconfig option, which is sensibly bounded, so that
>> platform developers may choose where to place this compromise.
>> Keep default values as new minimums.
>>
>> This patch makes sure that now powerpc mmap arch_mmap_rnd() approach
>> is similar to other ARCHs like x86, arm64 and arm.
>
> Thanks for looking at this, it's been on my TODO for a while.
>
> I have a half completed version locally, but never got around to testing
> it thoroughly.

Sure :)

>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index a8ee573fe610..b4a843f68705 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -22,6 +22,38 @@ config MMU
>>       bool
>>       default y
>>
>> +config ARCH_MMAP_RND_BITS_MIN
>> +       default 5 if PPC_256K_PAGES && 32BIT
>> +       default 12 if PPC_256K_PAGES && 64BIT
>> +       default 7 if PPC_64K_PAGES && 32BIT
>> +       default 14 if PPC_64K_PAGES && 64BIT
>> +       default 9 if PPC_16K_PAGES && 32BIT
>> +       default 16 if PPC_16K_PAGES && 64BIT
>> +       default 11 if PPC_4K_PAGES && 32BIT
>> +       default 18 if PPC_4K_PAGES && 64BIT
>> +
>> +# max bits determined by the following formula:
>> +#  VA_BITS - PAGE_SHIFT - 4
>> +#  for e.g for 64K page and 64BIT = 48 - 16 - 4 = 28
>> +config ARCH_MMAP_RND_BITS_MAX
>> +       default 10 if PPC_256K_PAGES && 32BIT
>> +       default 26 if PPC_256K_PAGES && 64BIT
>> +       default 12 if PPC_64K_PAGES && 32BIT
>> +       default 28 if PPC_64K_PAGES && 64BIT
>> +       default 14 if PPC_16K_PAGES && 32BIT
>> +       default 30 if PPC_16K_PAGES && 64BIT
>> +       default 16 if PPC_4K_PAGES && 32BIT
>> +       default 32 if PPC_4K_PAGES && 64BIT
>> +
>> +config ARCH_MMAP_RND_COMPAT_BITS_MIN
>> +       default 5 if PPC_256K_PAGES
>> +       default 7 if PPC_64K_PAGES
>> +       default 9 if PPC_16K_PAGES
>> +       default 11
>> +
>> +config ARCH_MMAP_RND_COMPAT_BITS_MAX
>> +       default 16
>> +
>
> This is what I have below, which is a bit neater I think because each
> value is only there once (by defaulting to the COMPAT value).
>
> My max values are different to yours, I don't really remember why I
> chose those values, so we can argue about which is right.

I am not sure how you derived these values, but I am not sure there
should be differences between 64-BIT x86/ARM64 and PPC values for the
MAX values.

>
> +config ARCH_MMAP_RND_BITS_MIN
> +       # On 64-bit up to 1G of address space (2^30)
> +       default 12 if 64BIT && PPC_256K_PAGES   # 256K (2^18), = 30 - 18 = 12
> +       default 14 if 64BIT && PPC_64K_PAGES    # 64K  (2^16), = 30 - 16 = 14
> +       default 16 if 64BIT && PPC_16K_PAGES    # 16K  (2^14), = 30 - 14 = 16
> +       default 18 if 64BIT                     # 4K   (2^12), = 30 - 12 = 18
> +       default ARCH_MMAP_RND_COMPAT_BITS_MIN
> +
> +config ARCH_MMAP_RND_BITS_MAX
> +       # On 64-bit up to 32T of address space (2^45)
> +       default 27 if 64BIT && PPC_256K_PAGES   # 256K (2^18), = 45 - 18 = 27
> +       default 29 if 64BIT && PPC_64K_PAGES    # 64K  (2^16), = 45 - 16 = 29
> +       default 31 if 64BIT && PPC_16K_PAGES    # 16K  (2^14), = 45 - 14 = 31
> +       default 33 if 64BIT                     # 4K   (2^12), = 45 - 12 = 33
> +       default ARCH_MMAP_RND_COMPAT_BITS_MAX
> +
> +config ARCH_MMAP_RND_COMPAT_BITS_MIN
> +       # Up to 8MB of address space (2^23)
> +       default 5 if PPC_256K_PAGES             # 256K (2^18), = 23 - 18 = 5
> +       default 7 if PPC_64K_PAGES              # 64K  (2^16), = 23 - 16 = 7
> +       default 9 if PPC_16K_PAGES              # 16K  (2^14), = 23 - 14 = 9
> +       default 11                              # 4K   (2^12), = 23 - 12 = 11
> +
> +config ARCH_MMAP_RND_COMPAT_BITS_MAX
> +       # Up to 2G of address space (2^31)
> +       default 13 if PPC_256K_PAGES            # 256K (2^18), = 31 - 18 = 13
> +       default 15 if PPC_64K_PAGES             # 64K  (2^16), = 31 - 16 = 15
> +       default 17 if PPC_16K_PAGES             # 16K  (2^14), = 31 - 14 = 17
> +       default 19                              # 4K   (2^12), = 31 - 12 = 19
>
>
>
>> diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c
>> index 2f1e44362198..babf59faab3b 100644
>> --- a/arch/powerpc/mm/mmap.c
>> +++ b/arch/powerpc/mm/mmap.c
>> @@ -60,11 +60,12 @@ unsigned long arch_mmap_rnd(void)
>>  {
>>       unsigned long rnd;
>>
>> -     /* 8MB for 32bit, 1GB for 64bit */
>> +#ifdef CONFIG_COMPAT
>>       if (is_32bit_task())
>> -             rnd = get_random_long() % (1<<(23-PAGE_SHIFT));
>> +             rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
>>       else
>> -             rnd = get_random_long() % (1UL<<(30-PAGE_SHIFT));
>> +#endif
>> +             rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
>
> I also have what I think is a better hunk for that:
>
>  unsigned long arch_mmap_rnd(void)
>  {
> -       unsigned long rnd;
> +       unsigned long shift, rnd;
>
> -       /* 8MB for 32bit, 1GB for 64bit */
> +       shift = mmap_rnd_bits;
> +#ifdef CONFIG_COMPAT
>         if (is_32bit_task())
> -               rnd = (unsigned long)get_random_int() % (1<<(23-PAGE_SHIFT));
> -       else
> -               rnd = (unsigned long)get_random_int() % (1<<(30-PAGE_SHIFT));
> +               shift = mmap_rnd_compat_bits;
> +#endif
> +
> +       rnd = (unsigned long)get_random_int() % (1 << shift);
>
> But I'm just nit picking I guess :)
>

I am trying to reuse the existing hunk available for x86 MMAP randomization.
So I am not sure if we really need the hunk mentioned above.

I think we can get this applied to upstream unless someone sees a
breakage on their platform (I have tested this on PPC64 and PPC64LE
setups at my end, but there might be some aberrations on some custom
PPC platforms)

Regards,
Bhupesh