[1/2] powerpc: mm: support ARCH_MMAP_RND_BITS
diff mbox

Message ID 1486014168-1279-2-git-send-email-bhsharma@redhat.com
State New
Headers show

Commit Message

Bhupesh Sharma Feb. 2, 2017, 5:42 a.m. UTC
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.

Cc: Alexander Graf <agraf@suse.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Alistair Popple <alistair@popple.id.au>
Cc: Matt Porter <mporter@kernel.crashing.org>
Cc: Vitaly Bordug <vitb@kernel.crashing.org>
Cc: Scott Wood <oss@buserror.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Daniel Cashman <dcashman@android.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
---
 arch/powerpc/Kconfig   | 34 ++++++++++++++++++++++++++++++++++
 arch/powerpc/mm/mmap.c |  7 ++++---
 2 files changed, 38 insertions(+), 3 deletions(-)

Comments

Michael Ellerman Feb. 2, 2017, 10:23 a.m. UTC | #1
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.

> 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.

+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 :)

cheers
Balbir Singh Feb. 2, 2017, 12:22 p.m. UTC | #2
On Thu, Feb 02, 2017 at 09:23:33PM +1100, Michael Ellerman wrote:
> +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)

I thought it was 64T, TASK_SIZE_USER64 is 2^46?

<snip>

> 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 :)
>

No.. the version above is nicer IMHO

Balbir
Kees Cook Feb. 2, 2017, 2:25 p.m. UTC | #3
On Wed, Feb 1, 2017 at 9:42 PM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> 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.
>
> Cc: Alexander Graf <agraf@suse.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Alistair Popple <alistair@popple.id.au>
> Cc: Matt Porter <mporter@kernel.crashing.org>
> Cc: Vitaly Bordug <vitb@kernel.crashing.org>
> Cc: Scott Wood <oss@buserror.net>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Daniel Cashman <dcashman@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
> ---
>  arch/powerpc/Kconfig   | 34 ++++++++++++++++++++++++++++++++++
>  arch/powerpc/mm/mmap.c |  7 ++++---
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> 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
> +
>  config HAVE_SETUP_PER_CPU_AREA
>         def_bool PPC64
>
> @@ -100,6 +132,8 @@ config PPC
>         select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>         select HAVE_KPROBES
>         select HAVE_ARCH_KGDB
> +       select HAVE_ARCH_MMAP_RND_BITS
> +       select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>         select HAVE_KRETPROBES
>         select HAVE_ARCH_TRACEHOOK
>         select HAVE_MEMBLOCK
> 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);
>
>         return rnd << PAGE_SHIFT;
>  }

Awesome! This looks good to me based on my earlier analysis.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees
Bhupesh Sharma Feb. 2, 2017, 6:04 p.m. UTC | #4
Hi Kees,

On Thu, Feb 2, 2017 at 7:55 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 1, 2017 at 9:42 PM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
>> 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.
>>
>> Cc: Alexander Graf <agraf@suse.com>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Anatolij Gustschin <agust@denx.de>
>> Cc: Alistair Popple <alistair@popple.id.au>
>> Cc: Matt Porter <mporter@kernel.crashing.org>
>> Cc: Vitaly Bordug <vitb@kernel.crashing.org>
>> Cc: Scott Wood <oss@buserror.net>
>> Cc: Kumar Gala <galak@kernel.crashing.org>
>> Cc: Daniel Cashman <dcashman@android.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
>> ---
>>  arch/powerpc/Kconfig   | 34 ++++++++++++++++++++++++++++++++++
>>  arch/powerpc/mm/mmap.c |  7 ++++---
>>  2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> 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
>> +
>>  config HAVE_SETUP_PER_CPU_AREA
>>         def_bool PPC64
>>
>> @@ -100,6 +132,8 @@ config PPC
>>         select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>         select HAVE_KPROBES
>>         select HAVE_ARCH_KGDB
>> +       select HAVE_ARCH_MMAP_RND_BITS
>> +       select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>>         select HAVE_KRETPROBES
>>         select HAVE_ARCH_TRACEHOOK
>>         select HAVE_MEMBLOCK
>> 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);
>>
>>         return rnd << PAGE_SHIFT;
>>  }
>
> Awesome! This looks good to me based on my earlier analysis.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Many thanks.
Regards,
Bhupesh
Bhupesh Sharma Feb. 2, 2017, 6:14 p.m. UTC | #5
HI Balbir,

On Thu, Feb 2, 2017 at 2:41 PM, Balbir Singh <bsingharora@gmail.com> wrote:
>> @@ -100,6 +132,8 @@ config PPC
>>       select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>       select HAVE_KPROBES
>>       select HAVE_ARCH_KGDB
>> +     select HAVE_ARCH_MMAP_RND_BITS
>> +     select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>
> COMPAT is on for ppc64 by default, so we'll end up with COMPAT_BITS same
> as before all the time.


No, actually the 'ARCH_MMAP_RND_COMPAT_BITS'  values can be changed
after boot using the '/proc/sys/vm/mmap_rnd_compat_bits' tunable:

http://lxr.free-electrons.com/source/arch/Kconfig#L624

Regards,
Bhupesh
Michael Ellerman Feb. 2, 2017, 11:59 p.m. UTC | #6
Balbir Singh <bsingharora@gmail.com> writes:

> On Thu, Feb 02, 2017 at 09:23:33PM +1100, Michael Ellerman wrote:
>> +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)
>
> I thought it was 64T, TASK_SIZE_USER64 is 2^46?

The virtual address space is 64T. The comment is talking about how much
can be taking up by the randomisation factor.

cheers
Bhupesh Sharma Feb. 8, 2017, 12:53 p.m. UTC | #7
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
Michael Ellerman Feb. 10, 2017, 11:01 a.m. UTC | #8
Bhupesh Sharma <bhsharma@redhat.com> writes:

> 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.

But your values *are* different to x86 and arm64.

And why would they be the same anyway? x86 has a 47 bit address space,
64-bit powerpc is 46 bits, and arm64 is configurable from 36 to 48 bits.

So your calculations above using VA_BITS = 48 should be using 46 bits.

But if you fixed that, your formula basically gives 1/16th of the
address space as the maximum range. Why is that the right amount?

x86 uses 1/8th, and arm64 uses a mixture of 1/8th and 1/32nd (though
those might be bugs).

My values were more liberal, giving up to half the address space for 32
& 64-bit. Maybe that's too generous, but my rationale was it's up to the
sysadmin to tweak the values and they get to keep the pieces if it
breaks.

>> +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

I played with my values a bit and allowing 32T is a little bit nuts. It
means you can actually end up with the adjusted ET_DYN_BASE *above* 32T,
followed by the heap growing up, and the mmap base *below* 32T, growing
down. Which is kinda fun, but definitely breaks a lot of assumptions.

So limiting it to a max of 16T is probably more sensible.

Anyway late here, will think about it some more over the weekend.

cheers
Bhupesh Sharma Feb. 10, 2017, 11:11 a.m. UTC | #9
On Fri, Feb 10, 2017 at 4:31 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Bhupesh Sharma <bhsharma@redhat.com> writes:
>
>> 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.
>
> But your values *are* different to x86 and arm64.
>
> And why would they be the same anyway? x86 has a 47 bit address space,
> 64-bit powerpc is 46 bits, and arm64 is configurable from 36 to 48 bits.
>
> So your calculations above using VA_BITS = 48 should be using 46 bits.
>
> But if you fixed that, your formula basically gives 1/16th of the
> address space as the maximum range. Why is that the right amount?
>
> x86 uses 1/8th, and arm64 uses a mixture of 1/8th and 1/32nd (though
> those might be bugs).
>
> My values were more liberal, giving up to half the address space for 32
> & 64-bit. Maybe that's too generous, but my rationale was it's up to the
> sysadmin to tweak the values and they get to keep the pieces if it
> breaks.

I am not sure why would one want to use more than the practical limits
of 1/8th used by x86 - this causes additional burden of address space
fragmentation.

So we need to balance between the randomness increase and the address
space fragmentation.

>>> +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
>
> I played with my values a bit and allowing 32T is a little bit nuts. It
> means you can actually end up with the adjusted ET_DYN_BASE *above* 32T,
> followed by the heap growing up, and the mmap base *below* 32T, growing
> down. Which is kinda fun, but definitely breaks a lot of assumptions.
>
> So limiting it to a max of 16T is probably more sensible.
>
> Anyway late here, will think about it some more over the weekend.

A user is always free to tweak the maximum values via specific Kconfig
+ defconfig combinations for their platforms, but why have such large
max values as default for say a embedded PPC64 board which only
supports say 16GB of DDR.

A default max of 33bits for such platforms might be an overkill, while
it might be fine for servers which might have greater DDR
availability.

Regards,
Bhupesh
Bhupesh Sharma Feb. 16, 2017, 4:49 a.m. UTC | #10
Hi Michael,

On Fri, Feb 10, 2017 at 4:41 PM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> On Fri, Feb 10, 2017 at 4:31 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Bhupesh Sharma <bhsharma@redhat.com> writes:
>>
>>> 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.
>>
>> But your values *are* different to x86 and arm64.
>>
>> And why would they be the same anyway? x86 has a 47 bit address space,
>> 64-bit powerpc is 46 bits, and arm64 is configurable from 36 to 48 bits.
>>
>> So your calculations above using VA_BITS = 48 should be using 46 bits.
>>
>> But if you fixed that, your formula basically gives 1/16th of the
>> address space as the maximum range. Why is that the right amount?
>>
>> x86 uses 1/8th, and arm64 uses a mixture of 1/8th and 1/32nd (though
>> those might be bugs).
>>
>> My values were more liberal, giving up to half the address space for 32
>> & 64-bit. Maybe that's too generous, but my rationale was it's up to the
>> sysadmin to tweak the values and they get to keep the pieces if it
>> breaks.
>
> I am not sure why would one want to use more than the practical limits
> of 1/8th used by x86 - this causes additional burden of address space
> fragmentation.
>
> So we need to balance between the randomness increase and the address
> space fragmentation.
>
>>>> +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
>>
>> I played with my values a bit and allowing 32T is a little bit nuts. It
>> means you can actually end up with the adjusted ET_DYN_BASE *above* 32T,
>> followed by the heap growing up, and the mmap base *below* 32T, growing
>> down. Which is kinda fun, but definitely breaks a lot of assumptions.
>>
>> So limiting it to a max of 16T is probably more sensible.
>>
>> Anyway late here, will think about it some more over the weekend.
>
> A user is always free to tweak the maximum values via specific Kconfig
> + defconfig combinations for their platforms, but why have such large
> max values as default for say a embedded PPC64 board which only
> supports say 16GB of DDR.
>
> A default max of 33bits for such platforms might be an overkill, while
> it might be fine for servers which might have greater DDR
> availability.

Ping.

Regards,
Bhupesh
Bhupesh Sharma Feb. 24, 2017, 7:32 a.m. UTC | #11
Hi Michael,

On Thu, Feb 16, 2017 at 10:19 AM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> Hi Michael,
>
> On Fri, Feb 10, 2017 at 4:41 PM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
>> On Fri, Feb 10, 2017 at 4:31 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> Bhupesh Sharma <bhsharma@redhat.com> writes:
>>>
>>>> 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.
>>>
>>> But your values *are* different to x86 and arm64.
>>>
>>> And why would they be the same anyway? x86 has a 47 bit address space,
>>> 64-bit powerpc is 46 bits, and arm64 is configurable from 36 to 48 bits.
>>>
>>> So your calculations above using VA_BITS = 48 should be using 46 bits.
>>>
>>> But if you fixed that, your formula basically gives 1/16th of the
>>> address space as the maximum range. Why is that the right amount?
>>>
>>> x86 uses 1/8th, and arm64 uses a mixture of 1/8th and 1/32nd (though
>>> those might be bugs).
>>>
>>> My values were more liberal, giving up to half the address space for 32
>>> & 64-bit. Maybe that's too generous, but my rationale was it's up to the
>>> sysadmin to tweak the values and they get to keep the pieces if it
>>> breaks.
>>
>> I am not sure why would one want to use more than the practical limits
>> of 1/8th used by x86 - this causes additional burden of address space
>> fragmentation.
>>
>> So we need to balance between the randomness increase and the address
>> space fragmentation.
>>
>>>>> +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
>>>
>>> I played with my values a bit and allowing 32T is a little bit nuts. It
>>> means you can actually end up with the adjusted ET_DYN_BASE *above* 32T,
>>> followed by the heap growing up, and the mmap base *below* 32T, growing
>>> down. Which is kinda fun, but definitely breaks a lot of assumptions.
>>>
>>> So limiting it to a max of 16T is probably more sensible.
>>>
>>> Anyway late here, will think about it some more over the weekend.
>>
>> A user is always free to tweak the maximum values via specific Kconfig
>> + defconfig combinations for their platforms, but why have such large
>> max values as default for say a embedded PPC64 board which only
>> supports say 16GB of DDR.
>>
>> A default max of 33bits for such platforms might be an overkill, while
>> it might be fine for servers which might have greater DDR
>> availability.
>
> Ping.
>

Any updates on this?

Regards,
Bhupesh
Michael Ellerman Feb. 24, 2017, 9:53 a.m. UTC | #12
On 24 February 2017 6:32:13 pm AEDT, Bhupesh Sharma <bhsharma@redhat.com> wrote:
>Hi Michael,
>
>On Thu, Feb 16, 2017 at 10:19 AM, Bhupesh Sharma <bhsharma@redhat.com>
>wrote:
>> Hi Michael,
>>
>> On Fri, Feb 10, 2017 at 4:41 PM, Bhupesh Sharma <bhsharma@redhat.com>
>wrote:
>>> On Fri, Feb 10, 2017 at 4:31 PM, Michael Ellerman
><mpe@ellerman.id.au> wrote:
>>>> Bhupesh Sharma <bhsharma@redhat.com> writes:
>>>>
>>>>> 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.
>>>>
>>>> But your values *are* different to x86 and arm64.
>>>>
>>>> And why would they be the same anyway? x86 has a 47 bit address
>space,
>>>> 64-bit powerpc is 46 bits, and arm64 is configurable from 36 to 48
>bits.
>>>>
>>>> So your calculations above using VA_BITS = 48 should be using 46
>bits.
>>>>
>>>> But if you fixed that, your formula basically gives 1/16th of the
>>>> address space as the maximum range. Why is that the right amount?
>>>>
>>>> x86 uses 1/8th, and arm64 uses a mixture of 1/8th and 1/32nd
>(though
>>>> those might be bugs).
>>>>
>>>> My values were more liberal, giving up to half the address space
>for 32
>>>> & 64-bit. Maybe that's too generous, but my rationale was it's up
>to the
>>>> sysadmin to tweak the values and they get to keep the pieces if it
>>>> breaks.
>>>
>>> I am not sure why would one want to use more than the practical
>limits
>>> of 1/8th used by x86 - this causes additional burden of address
>space
>>> fragmentation.
>>>
>>> So we need to balance between the randomness increase and the
>address
>>> space fragmentation.
>>>
>>>>>> +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
>>>>
>>>> I played with my values a bit and allowing 32T is a little bit
>nuts. It
>>>> means you can actually end up with the adjusted ET_DYN_BASE *above*
>32T,
>>>> followed by the heap growing up, and the mmap base *below* 32T,
>growing
>>>> down. Which is kinda fun, but definitely breaks a lot of
>assumptions.
>>>>
>>>> So limiting it to a max of 16T is probably more sensible.
>>>>
>>>> Anyway late here, will think about it some more over the weekend.
>>>
>>> A user is always free to tweak the maximum values via specific
>Kconfig
>>> + defconfig combinations for their platforms, but why have such
>large
>>> max values as default for say a embedded PPC64 board which only
>>> supports say 16GB of DDR.
>>>
>>> A default max of 33bits for such platforms might be an overkill,
>while
>>> it might be fine for servers which might have greater DDR
>>> availability.
>>
>> Ping.
>>
>
>Any updates on this?

It missed 4.11, we'll get something in for 4.12.

Cheers

Patch
diff mbox

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
+
 config HAVE_SETUP_PER_CPU_AREA
 	def_bool PPC64
 
@@ -100,6 +132,8 @@  config PPC
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
 	select HAVE_KPROBES
 	select HAVE_ARCH_KGDB
+	select HAVE_ARCH_MMAP_RND_BITS
+	select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
 	select HAVE_KRETPROBES
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_MEMBLOCK
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);
 
 	return rnd << PAGE_SHIFT;
 }