HI Michael,
On Thu, Feb 2, 2017 at 3:53 PM, Michael Ellerman <m...@ellerman.id.au> wrote:
> Bhupesh Sharma <bhsha...@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
