| Message ID | 1486014168-1279-2-git-send-email-bhsharma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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; }
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(-)