[PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Daniel Cashman]

From: dcashman <dcashman@google.com>

ASLR currently only uses 8 bits to generate the random offset for the
mmap base address on 32 bit architectures. This value was chosen to
prevent a poorly chosen value from dividing the address space in such
a way as to prevent large allocations. This may not be an issue on all
platforms. Allow the specification of a minimum number of bits so that
platforms desiring greater ASLR protection may determine where to place
the trade-off.

Signed-off-by: Daniel Cashman <dcashman@google.com>
---
Changes in v2:
  - Added HAVE_ARCH_MMAP_RND_BITS as Kconfig boolean selector.
  - Moved ARCH_MMAP_RND_BITS_MIN, ARCH_MMAP_RND_BITS_MAX, and
  ARCH_MMAP_RND_BITS declarations to arch/Kconfig instead of relying
  soley on arch-specific Kconfigs.
  - Moved definition of mmap_rnd_bits_min, mmap_rnd_bits_max and
  mmap_rnd_bits to mm/mmap.c instead of relying solely on arch-specific
  code.

 Documentation/sysctl/kernel.txt | 14 ++++++++++++++
 arch/Kconfig                    | 29 +++++++++++++++++++++++++++++
 include/linux/mm.h              |  6 ++++++
 kernel/sysctl.c                 | 11 +++++++++++
 mm/mmap.c                       |  6 ++++++
 5 files changed, 66 insertions(+)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 6fccb69..0d4ca53 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -41,6 +41,7 @@ show up in /proc/sys/kernel:
 - kptr_restrict
 - kstack_depth_to_print       [ X86 only ]
 - l2cr                        [ PPC only ]
+- mmap_rnd_bits
 - modprobe                    ==> Documentation/debugging-modules.txt
 - modules_disabled
 - msg_next_id		      [ sysv ipc ]
@@ -391,6 +392,19 @@ This flag controls the L2 cache of G3 processor boards. If
 
 ==============================================================
 
+mmap_rnd_bits:
+
+This value can be used to select the number of bits to use to
+determine the random offset to the base address of vma regions
+resulting from mmap allocations on architectures which support
+tuning address space randomization.  This value will be bounded
+by the architecture's minimum and maximum supported values.
+
+This value can be changed after boot using the
+/proc/sys/kernel/mmap_rnd_bits tunable
+
+==============================================================
+
 modules_disabled:
 
 A toggle value indicating if modules are allowed to be loaded
diff --git a/arch/Kconfig b/arch/Kconfig
index 4e949e5..2133973 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -511,6 +511,35 @@ config ARCH_HAS_ELF_RANDOMIZE
 	  - arch_mmap_rnd()
 	  - arch_randomize_brk()
 
+config HAVE_ARCH_MMAP_RND_BITS
+	bool
+	help
+	  An arch should select this symbol if it supports setting a variable
+	  number of bits for use in establishing the base address for mmap
+	  allocations and provides values for both:
+	  - ARCH_MMAP_RND_BITS_MIN
+	  - ARCH_MMAP_RND_BITS_MAX
+
+config ARCH_MMAP_RND_BITS_MIN
+	int
+
+config ARCH_MMAP_RND_BITS_MAX
+	int
+
+config ARCH_MMAP_RND_BITS
+	int "Number of bits to use for ASLR of mmap base address" if EXPERT
+	range ARCH_MMAP_RND_BITS_MIN ARCH_MMAP_RND_BITS_MAX
+	default ARCH_MMAP_RND_BITS_MIN
+	depends on HAVE_ARCH_MMAP_RND_BITS
+	help
+	  This value can be used to select the number of bits to use to
+	  determine the random offset to the base address of vma regions
+	  resulting from mmap allocations. This value will be bounded
+	  by the architecture's minimum and maximum supported values.
+
+	  This value can be changed after boot using the
+	  /proc/sys/kernel/mmap_rnd_bits tunable
+
 config HAVE_COPY_THREAD_TLS
 	bool
 	help
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 80001de..ee209c1 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -51,6 +51,12 @@ extern int sysctl_legacy_va_layout;
 #define sysctl_legacy_va_layout 0
 #endif
 
+#ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS
+extern int mmap_rnd_bits_min;
+extern int mmap_rnd_bits_max;
+extern int mmap_rnd_bits;
+#endif
+
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/processor.h>
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e69201d..276da8b 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1139,6 +1139,17 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= timer_migration_handler,
 	},
 #endif
+#ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS
+	{
+		.procname	= "mmap_rnd_bits",
+		.data		= &mmap_rnd_bits,
+		.maxlen		= sizeof(mmap_rnd_bits),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &mmap_rnd_bits_min,
+		.extra2		= &mmap_rnd_bits_max,
+	},
+#endif
 	{ }
 };
 
diff --git a/mm/mmap.c b/mm/mmap.c
index 79bcc9f..264aa8e 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -58,6 +58,12 @@
 #define arch_rebalance_pgtables(addr, len)		(addr)
 #endif
 
+#ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS
+int mmap_rnd_bits_min = CONFIG_ARCH_MMAP_RND_BITS_MIN;
+int mmap_rnd_bits_max = CONFIG_ARCH_MMAP_RND_BITS_MAX;
+int mmap_rnd_bits = CONFIG_ARCH_MMAP_RND_BITS;
+#endif
+
 static void unmap_region(struct mm_struct *mm,
 		struct vm_area_struct *vma, struct vm_area_struct *prev,
 		unsigned long start, unsigned long end);
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Daniel Cashman]

From: dcashman <dcashman@google.com>

arm: arch_mmap_rnd() uses a hard-code value of 8 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 8 as the minimum acceptable value.

Signed-off-by: Daniel Cashman <dcashman@google.com>
---
Changes in v2:
  - Changed arch/arm/Kconfig and arch/arm/mm/mmap.c to reflect changes
  in [PATCH v2 1/2], specifically the movement of variables to global
  rather than arch-specific files.

 arch/arm/Kconfig   | 10 ++++++++++
 arch/arm/mm/mmap.c |  3 +--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 639411f..47d7561 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -35,6 +35,7 @@ config ARM
 	select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6
 	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32
 	select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32
+	select HAVE_ARCH_MMAP_RND_BITS
 	select HAVE_ARCH_SECCOMP_FILTER if (AEABI && !OABI_COMPAT)
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_BPF_JIT
@@ -306,6 +307,15 @@ config MMU
 	  Select if you want MMU-based virtualised addressing space
 	  support by paged memory management. If unsure, say 'Y'.
 
+config ARCH_MMAP_RND_BITS_MIN
+	default 8
+
+config ARCH_MMAP_RND_BITS_MAX
+	default 14 if MMU && PAGE_OFFSET=0x40000000
+	default 15 if MMU && PAGE_OFFSET=0x80000000
+	default 16 if MMU
+	default 8
+
 #
 # The "ARM system type" choice list is ordered alphabetically by option
 # text.  Please add new entries in the option alphabetic order.
diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index 407dc78..c938693 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -173,8 +173,7 @@ unsigned long arch_mmap_rnd(void)
 {
 	unsigned long rnd;
 
-	/* 8 bits of randomness in 20 address space bits */
-	rnd = (unsigned long)get_random_int() % (1 << 8);
+	rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_bits);
 
 	return rnd << PAGE_SHIFT;
 }
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Kees Cook]

On Tue, Nov 3, 2015 at 10:10 AM, Daniel Cashman <dcashman@android.com> wrote:
> From: dcashman <dcashman@google.com>
>
> ASLR currently only uses 8 bits to generate the random offset for the
> mmap base address on 32 bit architectures. This value was chosen to
> prevent a poorly chosen value from dividing the address space in such
> a way as to prevent large allocations. This may not be an issue on all
> platforms. Allow the specification of a minimum number of bits so that
> platforms desiring greater ASLR protection may determine where to place
> the trade-off.
>
> Signed-off-by: Daniel Cashman <dcashman@google.com>

I like this, thanks for working on it!

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

We might end up in situations on some architectures where mappings
might end up crashing into each other, but I think that'll be a
per-arch concern. Being able to set this at all is a great
improvement.

Thanks!

-Kees

> ---
> Changes in v2:
>   - Added HAVE_ARCH_MMAP_RND_BITS as Kconfig boolean selector.
>   - Moved ARCH_MMAP_RND_BITS_MIN, ARCH_MMAP_RND_BITS_MAX, and
>   ARCH_MMAP_RND_BITS declarations to arch/Kconfig instead of relying
>   soley on arch-specific Kconfigs.
>   - Moved definition of mmap_rnd_bits_min, mmap_rnd_bits_max and
>   mmap_rnd_bits to mm/mmap.c instead of relying solely on arch-specific
>   code.
>
>  Documentation/sysctl/kernel.txt | 14 ++++++++++++++
>  arch/Kconfig                    | 29 +++++++++++++++++++++++++++++
>  include/linux/mm.h              |  6 ++++++
>  kernel/sysctl.c                 | 11 +++++++++++
>  mm/mmap.c                       |  6 ++++++
>  5 files changed, 66 insertions(+)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 6fccb69..0d4ca53 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -41,6 +41,7 @@ show up in /proc/sys/kernel:
>  - kptr_restrict
>  - kstack_depth_to_print       [ X86 only ]
>  - l2cr                        [ PPC only ]
> +- mmap_rnd_bits
>  - modprobe                    ==> Documentation/debugging-modules.txt
>  - modules_disabled
>  - msg_next_id                [ sysv ipc ]
> @@ -391,6 +392,19 @@ This flag controls the L2 cache of G3 processor boards. If
>
>  ==============================================================
>
> +mmap_rnd_bits:
> +
> +This value can be used to select the number of bits to use to
> +determine the random offset to the base address of vma regions
> +resulting from mmap allocations on architectures which support
> +tuning address space randomization.  This value will be bounded
> +by the architecture's minimum and maximum supported values.
> +
> +This value can be changed after boot using the
> +/proc/sys/kernel/mmap_rnd_bits tunable
> +
> +==============================================================
> +
>  modules_disabled:
>
>  A toggle value indicating if modules are allowed to be loaded
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 4e949e5..2133973 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -511,6 +511,35 @@ config ARCH_HAS_ELF_RANDOMIZE
>           - arch_mmap_rnd()
>           - arch_randomize_brk()
>
> +config HAVE_ARCH_MMAP_RND_BITS
> +       bool
> +       help
> +         An arch should select this symbol if it supports setting a variable
> +         number of bits for use in establishing the base address for mmap
> +         allocations and provides values for both:
> +         - ARCH_MMAP_RND_BITS_MIN
> +         - ARCH_MMAP_RND_BITS_MAX
> +
> +config ARCH_MMAP_RND_BITS_MIN
> +       int
> +
> +config ARCH_MMAP_RND_BITS_MAX
> +       int
> +
> +config ARCH_MMAP_RND_BITS
> +       int "Number of bits to use for ASLR of mmap base address" if EXPERT
> +       range ARCH_MMAP_RND_BITS_MIN ARCH_MMAP_RND_BITS_MAX
> +       default ARCH_MMAP_RND_BITS_MIN
> +       depends on HAVE_ARCH_MMAP_RND_BITS
> +       help
> +         This value can be used to select the number of bits to use to
> +         determine the random offset to the base address of vma regions
> +         resulting from mmap allocations. This value will be bounded
> +         by the architecture's minimum and maximum supported values.
> +
> +         This value can be changed after boot using the
> +         /proc/sys/kernel/mmap_rnd_bits tunable
> +
>  config HAVE_COPY_THREAD_TLS
>         bool
>         help
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 80001de..ee209c1 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -51,6 +51,12 @@ extern int sysctl_legacy_va_layout;
>  #define sysctl_legacy_va_layout 0
>  #endif
>
> +#ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS
> +extern int mmap_rnd_bits_min;
> +extern int mmap_rnd_bits_max;
> +extern int mmap_rnd_bits;
> +#endif
> +
>  #include <asm/page.h>
>  #include <asm/pgtable.h>
>  #include <asm/processor.h>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e69201d..276da8b 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1139,6 +1139,17 @@ static struct ctl_table kern_table[] = {
>                 .proc_handler   = timer_migration_handler,
>         },
>  #endif
> +#ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS
> +       {
> +               .procname       = "mmap_rnd_bits",
> +               .data           = &mmap_rnd_bits,
> +               .maxlen         = sizeof(mmap_rnd_bits),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dointvec_minmax,
> +               .extra1         = &mmap_rnd_bits_min,
> +               .extra2         = &mmap_rnd_bits_max,
> +       },
> +#endif
>         { }
>  };
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 79bcc9f..264aa8e 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -58,6 +58,12 @@
>  #define arch_rebalance_pgtables(addr, len)             (addr)
>  #endif
>
> +#ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS
> +int mmap_rnd_bits_min = CONFIG_ARCH_MMAP_RND_BITS_MIN;
> +int mmap_rnd_bits_max = CONFIG_ARCH_MMAP_RND_BITS_MAX;
> +int mmap_rnd_bits = CONFIG_ARCH_MMAP_RND_BITS;
> +#endif
> +
>  static void unmap_region(struct mm_struct *mm,
>                 struct vm_area_struct *vma, struct vm_area_struct *prev,
>                 unsigned long start, unsigned long end);
> --
> 2.6.0.rc2.230.g3dd15c0
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Kees Cook]

On Tue, Nov 3, 2015 at 10:10 AM, Daniel Cashman <dcashman@android.com> wrote:
> From: dcashman <dcashman@google.com>
>
> arm: arch_mmap_rnd() uses a hard-code value of 8 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 8 as the minimum acceptable value.
>
> Signed-off-by: Daniel Cashman <dcashman@google.com>

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

Russell, if you don't see any problems here, it might make sense not
to put this through the ARM patch tracker since it depends on the 1/2,
and I think x86 and arm64 (and possibly other arch) changes are coming
too.

> ---
> Changes in v2:
>   - Changed arch/arm/Kconfig and arch/arm/mm/mmap.c to reflect changes
>   in [PATCH v2 1/2], specifically the movement of variables to global
>   rather than arch-specific files.
>
>  arch/arm/Kconfig   | 10 ++++++++++
>  arch/arm/mm/mmap.c |  3 +--
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 639411f..47d7561 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -35,6 +35,7 @@ config ARM
>         select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6
>         select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32
>         select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32
> +       select HAVE_ARCH_MMAP_RND_BITS
>         select HAVE_ARCH_SECCOMP_FILTER if (AEABI && !OABI_COMPAT)
>         select HAVE_ARCH_TRACEHOOK
>         select HAVE_BPF_JIT
> @@ -306,6 +307,15 @@ config MMU
>           Select if you want MMU-based virtualised addressing space
>           support by paged memory management. If unsure, say 'Y'.
>
> +config ARCH_MMAP_RND_BITS_MIN
> +       default 8
> +
> +config ARCH_MMAP_RND_BITS_MAX
> +       default 14 if MMU && PAGE_OFFSET=0x40000000
> +       default 15 if MMU && PAGE_OFFSET=0x80000000
> +       default 16 if MMU
> +       default 8
> +
>  #
>  # The "ARM system type" choice list is ordered alphabetically by option
>  # text.  Please add new entries in the option alphabetic order.
> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
> index 407dc78..c938693 100644
> --- a/arch/arm/mm/mmap.c
> +++ b/arch/arm/mm/mmap.c
> @@ -173,8 +173,7 @@ unsigned long arch_mmap_rnd(void)
>  {
>         unsigned long rnd;
>
> -       /* 8 bits of randomness in 20 address space bits */
> -       rnd = (unsigned long)get_random_int() % (1 << 8);
> +       rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_bits);
>
>         return rnd << PAGE_SHIFT;
>  }

I like this getting pulled closer and closer to having arch_mmap_rnd()
be identical across all architectures, and then we can just pull it
out and leave the true variable: the entropy size.

Do you have patches for x86 and arm64?

-Kees

> --
> 2.6.0.rc2.230.g3dd15c0
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Russell King - ARM Linux]

On Tue, Nov 03, 2015 at 11:19:44AM -0800, Kees Cook wrote:
> On Tue, Nov 3, 2015 at 10:10 AM, Daniel Cashman <dcashman@android.com> wrote:
> > From: dcashman <dcashman@google.com>
> >
> > arm: arch_mmap_rnd() uses a hard-code value of 8 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 8 as the minimum acceptable value.
> >
> > Signed-off-by: Daniel Cashman <dcashman@google.com>
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> Russell, if you don't see any problems here, it might make sense not
> to put this through the ARM patch tracker since it depends on the 1/2,
> and I think x86 and arm64 (and possibly other arch) changes are coming
> too.

Yes, it looks sane, though I do wonder whether there should also be
a Kconfig option to allow archtectures to specify the default, instead
of the default always being the minimum randomisation.  I can see scope
to safely pushing our mmap randomness default to 12, especially on 3GB
setups, as we already have 11 bits of randomness on the sigpage and if
enabled, 13 bits on the heap.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Daniel Cashman]

On 11/03/2015 11:19 AM, Kees Cook wrote:
> Do you have patches for x86 and arm64? 

I was holding off on those until I could gauge upstream reception.  If
desired, I could put those together and add them as [PATCH 3/4] and
[PATCH 4/4].

Thank You,
Dan

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Kees Cook]

On Tue, Nov 3, 2015 at 2:39 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 03, 2015 at 11:19:44AM -0800, Kees Cook wrote:
>> On Tue, Nov 3, 2015 at 10:10 AM, Daniel Cashman <dcashman@android.com> wrote:
>> > From: dcashman <dcashman@google.com>
>> >
>> > arm: arch_mmap_rnd() uses a hard-code value of 8 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 8 as the minimum acceptable value.
>> >
>> > Signed-off-by: Daniel Cashman <dcashman@google.com>
>>
>> Acked-by: Kees Cook <keescook@chromium.org>
>>
>> Russell, if you don't see any problems here, it might make sense not
>> to put this through the ARM patch tracker since it depends on the 1/2,
>> and I think x86 and arm64 (and possibly other arch) changes are coming
>> too.
>
> Yes, it looks sane, though I do wonder whether there should also be
> a Kconfig option to allow archtectures to specify the default, instead
> of the default always being the minimum randomisation.  I can see scope
> to safely pushing our mmap randomness default to 12, especially on 3GB
> setups, as we already have 11 bits of randomness on the sigpage and if
> enabled, 13 bits on the heap.

My thinking is that the there shouldn't be a reason to ever have a
minimum that was below the default. I have no objection with it, but
it seems needless. Frankly minimum is "0", really, so I don't think it
makes much sense to have default != arch minimum. I actually view
"arch minimum" as "known good", so if we are happy with raising the
"known good" value, that should be the new minimum.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Kees Cook]

On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
> On 11/03/2015 11:19 AM, Kees Cook wrote:
>> Do you have patches for x86 and arm64?
>
> I was holding off on those until I could gauge upstream reception.  If
> desired, I could put those together and add them as [PATCH 3/4] and
> [PATCH 4/4].

If they're as trivial as I'm hoping, yeah, let's toss them in now. If
not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
too, but one or two of those have somewhat stranger calculations when
I looked, so their Kconfigs may not be as clean.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Andrew Morton]

On Tue,  3 Nov 2015 10:10:03 -0800 Daniel Cashman <dcashman@android.com> wrote:

> ASLR currently only uses 8 bits to generate the random offset for the
> mmap base address on 32 bit architectures. This value was chosen to
> prevent a poorly chosen value from dividing the address space in such
> a way as to prevent large allocations. This may not be an issue on all
> platforms. Allow the specification of a minimum number of bits so that
> platforms desiring greater ASLR protection may determine where to place
> the trade-off.

Can we please include a very good description of the motivation for this
change?  What is inadequate about the current code, what value does the
enhancement have to our users, what real-world problems are being solved,
etc.

Because all we have at present is "greater ASLR protection", which doesn't
really tell anyone anything.

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Tue,  3 Nov 2015 10:10:03 -0800 Daniel Cashman <dcashman@android.com> wrote:
>
>> ASLR currently only uses 8 bits to generate the random offset for the
>> mmap base address on 32 bit architectures. This value was chosen to
>> prevent a poorly chosen value from dividing the address space in such
>> a way as to prevent large allocations. This may not be an issue on all
>> platforms. Allow the specification of a minimum number of bits so that
>> platforms desiring greater ASLR protection may determine where to place
>> the trade-off.
>
> Can we please include a very good description of the motivation for this
> change?  What is inadequate about the current code, what value does the
> enhancement have to our users, what real-world problems are being solved,
> etc.
>
> Because all we have at present is "greater ASLR protection", which doesn't
> really tell anyone anything.

The description seemed clear to me.

More random bits, more entropy, more work needed to brute force.

8 bits only requires 256 tries (or a 1 in 256) chance to brute force
something.

We have seen in the last couple of months on Android how only having 8 bits
doesn't help much.

Each additional bit doubles the protection (and unfortunately also
increases fragmentation of the userspace address space).

Eric

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Andrew Morton]

On Tue, 03 Nov 2015 18:40:31 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Tue,  3 Nov 2015 10:10:03 -0800 Daniel Cashman <dcashman@android.com> wrote:
> >
> >> ASLR currently only uses 8 bits to generate the random offset for the
> >> mmap base address on 32 bit architectures. This value was chosen to
> >> prevent a poorly chosen value from dividing the address space in such
> >> a way as to prevent large allocations. This may not be an issue on all
> >> platforms. Allow the specification of a minimum number of bits so that
> >> platforms desiring greater ASLR protection may determine where to place
> >> the trade-off.
> >
> > Can we please include a very good description of the motivation for this
> > change?  What is inadequate about the current code, what value does the
> > enhancement have to our users, what real-world problems are being solved,
> > etc.
> >
> > Because all we have at present is "greater ASLR protection", which doesn't
> > really tell anyone anything.
> 
> The description seemed clear to me.
> 
> More random bits, more entropy, more work needed to brute force.
> 
> 8 bits only requires 256 tries (or a 1 in 256) chance to brute force
> something.

Of course, but that's not really very useful.

> We have seen in the last couple of months on Android how only having 8 bits
> doesn't help much.

Now THAT is important.  What happened here and how well does the
proposed fix improve things?  How much longer will a brute-force attack
take to succeed, with a particular set of kernel parameters?  Is the
new duration considered to be sufficiently long and if not, are there
alternative fixes we should be looking at?

Stuff like this.

> Each additional bit doubles the protection (and unfortunately also
> increases fragmentation of the userspace address space).

OK, so the benefit comes with a cost and people who are configuring
systems (and the people who are reviewing this patchset!) need to
understand the tradeoffs.  Please.

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Michal Hocko]

On Tue 03-11-15 10:10:03, Daniel Cashman wrote:
[...]
> +This value can be changed after boot using the
> +/proc/sys/kernel/mmap_rnd_bits tunable

Why is this not sitting in /proc/sys/vm/ where we already have
mmap_min_addr. These two sound like they should sit together, no?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Daniel Cashman]

On 11/3/15 3:18 PM, Kees Cook wrote:
> On Tue, Nov 3, 2015 at 2:39 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Tue, Nov 03, 2015 at 11:19:44AM -0800, Kees Cook wrote:
>>> On Tue, Nov 3, 2015 at 10:10 AM, Daniel Cashman <dcashman@android.com> wrote:
>>>> From: dcashman <dcashman@google.com>
>>>>
>>>> arm: arch_mmap_rnd() uses a hard-code value of 8 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 8 as the minimum acceptable value.
>>>>
>>>> Signed-off-by: Daniel Cashman <dcashman@google.com>
>>>
>>> Acked-by: Kees Cook <keescook@chromium.org>
>>>
>>> Russell, if you don't see any problems here, it might make sense not
>>> to put this through the ARM patch tracker since it depends on the 1/2,
>>> and I think x86 and arm64 (and possibly other arch) changes are coming
>>> too.
>>
>> Yes, it looks sane, though I do wonder whether there should also be
>> a Kconfig option to allow archtectures to specify the default, instead
>> of the default always being the minimum randomisation.  I can see scope
>> to safely pushing our mmap randomness default to 12, especially on 3GB
>> setups, as we already have 11 bits of randomness on the sigpage and if
>> enabled, 13 bits on the heap.
> 
> My thinking is that the there shouldn't be a reason to ever have a
> minimum that was below the default. I have no objection with it, but
> it seems needless. Frankly minimum is "0", really, so I don't think it
> makes much sense to have default != arch minimum. I actually view
> "arch minimum" as "known good", so if we are happy with raising the
> "known good" value, that should be the new minimum.

While I generally agree, the ability to specify a non-minimum arch
default could be useful if there is a small fraction which relies on
having a small value.  8 as the current minimum for arm made sense to me
since it has already been established as minimum in the current code.
It may be the case, as Russel has suggested for example, that we could
up the default to 12 for the vast majority of systems, but that 8 could
still be required for a select few.  In this case, our current solution
would have to leave the minimum at 8, and thus leave the default at 8
for all systems when 12 would be preferable. This patch allows those
systems to change that, of course, so the question becomes one of opt-in
vs opt-out for an increased amount of randomness if this situation occurred.

Both approaches seem reasonable to me.  Russel, if you'd still like the
ability to specify a non-minimum default, would establishing an
additional Kconfig variable, say ARCH_HAS_DEF_MMAP_RND_BITS, or simply
dropping the default line from the global config be preferable?

Thank You,
Dan

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Daniel Cashman]

On 11/3/15 3:21 PM, Kees Cook wrote:
> On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
>> On 11/03/2015 11:19 AM, Kees Cook wrote:
>>> Do you have patches for x86 and arm64?
>>
>> I was holding off on those until I could gauge upstream reception.  If
>> desired, I could put those together and add them as [PATCH 3/4] and
>> [PATCH 4/4].
> 
> If they're as trivial as I'm hoping, yeah, let's toss them in now. If
> not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
> too, but one or two of those have somewhat stranger calculations when
> I looked, so their Kconfigs may not be as clean.

Creating the patches should be simple, it's the choice of minimum and
maximum values for each architecture that I'd be most concerned about.
I'll put them together, though, and the ranges can be changed following
discussion with those more knowledgeable, if needed.  I also don't have
devices on which to test the PowerPC, MIPS and s390 changes, so I'll
need someone's help for that.

Thank You,
Dan

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Eric W. Biederman]

Michal Hocko <mhocko@kernel.org> writes:

> On Tue 03-11-15 10:10:03, Daniel Cashman wrote:
> [...]
>> +This value can be changed after boot using the
>> +/proc/sys/kernel/mmap_rnd_bits tunable
>
> Why is this not sitting in /proc/sys/vm/ where we already have
> mmap_min_addr. These two sound like they should sit together, no?

Ugh.  Yes.  Moving that file before it becomes part of the ABI sounds
like a good idea.  Daniel when you get around to v3 please move the
file.

Thank you,
Eric

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Daniel Cashman]

On 11/3/15 5:31 PM, Andrew Morton wrote:
> On Tue, 03 Nov 2015 18:40:31 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:
> 
>> Andrew Morton <akpm@linux-foundation.org> writes:
>>
>>> On Tue,  3 Nov 2015 10:10:03 -0800 Daniel Cashman <dcashman@android.com> wrote:
>>>
>>>> ASLR currently only uses 8 bits to generate the random offset for the
>>>> mmap base address on 32 bit architectures. This value was chosen to
>>>> prevent a poorly chosen value from dividing the address space in such
>>>> a way as to prevent large allocations. This may not be an issue on all
>>>> platforms. Allow the specification of a minimum number of bits so that
>>>> platforms desiring greater ASLR protection may determine where to place
>>>> the trade-off.
>>>
>>> Can we please include a very good description of the motivation for this
>>> change?  What is inadequate about the current code, what value does the
>>> enhancement have to our users, what real-world problems are being solved,
>>> etc.
>>>
>>> Because all we have at present is "greater ASLR protection", which doesn't
>>> really tell anyone anything.
>>
>> The description seemed clear to me.
>>
>> More random bits, more entropy, more work needed to brute force.
>>
>> 8 bits only requires 256 tries (or a 1 in 256) chance to brute force
>> something.
> 
> Of course, but that's not really very useful.
> 
>> We have seen in the last couple of months on Android how only having 8 bits
>> doesn't help much.
> 
> Now THAT is important.  What happened here and how well does the
> proposed fix improve things?  How much longer will a brute-force attack
> take to succeed, with a particular set of kernel parameters?  Is the
> new duration considered to be sufficiently long and if not, are there
> alternative fixes we should be looking at?
> 
> Stuff like this.
> 
>> Each additional bit doubles the protection (and unfortunately also
>> increases fragmentation of the userspace address space).
> 
> OK, so the benefit comes with a cost and people who are configuring
> systems (and the people who are reviewing this patchset!) need to
> understand the tradeoffs.  Please.

The direct motivation here was in response to the libstagefright
vulnerabilities that affected Android, specifically to information
provided by Google's project zero at:

http://googleprojectzero.blogspot.com/2015/09/stagefrightened.html

The attack there specifically used the limited randomness used in
generating the mmap base address as part of a brute-force-based exploit.
 In this particular case, the attack was against the mediaserver process
on Android, which was limited to respawning every 5 seconds, giving the
attacker an average expected success rate of defeating the mmap ASLR
after over 10 minutes (128 tries at 5 seconds each).  With change to the
maximum proposed value of 16 bits, this would change to over 45 hours
(32768 tries), which would make the user of such a system much more
likely to notice such an attack.

I understand the desire for this clarification, and will happily try to
improve the explanation for this change, especially so that those
considering use of this option understand the tradeoffs, but I also view
this as one particular hardening change which is a component of making
attacks such as these harder, rather than the only solution.  As for the
clarification itself, where would you like it?  I could include a cover
letter for this patch-set, elaborate more in the commit message itself,
add more to the Kconfig help description, or some combination of the above.

Thank You,
Dan

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Daniel Cashman]

On 11/4/15 11:21 AM, Eric W. Biederman wrote:
> Michal Hocko <mhocko@kernel.org> writes:
> 
>> On Tue 03-11-15 10:10:03, Daniel Cashman wrote:
>> [...]
>>> +This value can be changed after boot using the
>>> +/proc/sys/kernel/mmap_rnd_bits tunable
>>
>> Why is this not sitting in /proc/sys/vm/ where we already have
>> mmap_min_addr. These two sound like they should sit together, no?
> 
> Ugh.  Yes.  Moving that file before it becomes part of the ABI sounds
> like a good idea.  Daniel when you get around to v3 please move the
> file.

To answer the first question: it was put there because that's where
randomize_va_space is located, which seemed to me to be the
most-related/similar option.  That being said, moving it under vm works
too.  Will change for patch-set 3.

Thank You,
Dan

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Andrew Morton]

On Wed, 4 Nov 2015 11:31:25 -0800 Daniel Cashman <dcashman@android.com> wrote:

> As for the
> clarification itself, where would you like it?  I could include a cover
> letter for this patch-set, elaborate more in the commit message itself,
> add more to the Kconfig help description, or some combination of the above.

In either [0/n] or [x/x] changelog, please.  I routinely move the [0/n]
material into the [1/n] changelog anyway.

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Eric W. Biederman]

Daniel Cashman <dcashman@android.com> writes:

> On 11/3/15 5:31 PM, Andrew Morton wrote:
>> On Tue, 03 Nov 2015 18:40:31 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:
>> 
>>> Andrew Morton <akpm@linux-foundation.org> writes:
>>>
>>>> On Tue,  3 Nov 2015 10:10:03 -0800 Daniel Cashman <dcashman@android.com> wrote:
>>>>
>>>>> ASLR currently only uses 8 bits to generate the random offset for the
>>>>> mmap base address on 32 bit architectures. This value was chosen to
>>>>> prevent a poorly chosen value from dividing the address space in such
>>>>> a way as to prevent large allocations. This may not be an issue on all
>>>>> platforms. Allow the specification of a minimum number of bits so that
>>>>> platforms desiring greater ASLR protection may determine where to place
>>>>> the trade-off.
>>>>
>>>> Can we please include a very good description of the motivation for this
>>>> change?  What is inadequate about the current code, what value does the
>>>> enhancement have to our users, what real-world problems are being solved,
>>>> etc.
>>>>
>>>> Because all we have at present is "greater ASLR protection", which doesn't
>>>> really tell anyone anything.
>>>
>>> The description seemed clear to me.
>>>
>>> More random bits, more entropy, more work needed to brute force.
>>>
>>> 8 bits only requires 256 tries (or a 1 in 256) chance to brute force
>>> something.
>> 
>> Of course, but that's not really very useful.
>> 
>>> We have seen in the last couple of months on Android how only having 8 bits
>>> doesn't help much.
>> 
>> Now THAT is important.  What happened here and how well does the
>> proposed fix improve things?  How much longer will a brute-force attack
>> take to succeed, with a particular set of kernel parameters?  Is the
>> new duration considered to be sufficiently long and if not, are there
>> alternative fixes we should be looking at?
>> 
>> Stuff like this.
>> 
>>> Each additional bit doubles the protection (and unfortunately also
>>> increases fragmentation of the userspace address space).
>> 
>> OK, so the benefit comes with a cost and people who are configuring
>> systems (and the people who are reviewing this patchset!) need to
>> understand the tradeoffs.  Please.
>
> The direct motivation here was in response to the libstagefright
> vulnerabilities that affected Android, specifically to information
> provided by Google's project zero at:
>
> http://googleprojectzero.blogspot.com/2015/09/stagefrightened.html
>
> The attack there specifically used the limited randomness used in
> generating the mmap base address as part of a brute-force-based exploit.
>  In this particular case, the attack was against the mediaserver process
> on Android, which was limited to respawning every 5 seconds, giving the
> attacker an average expected success rate of defeating the mmap ASLR
> after over 10 minutes (128 tries at 5 seconds each).  With change to the
> maximum proposed value of 16 bits, this would change to over 45 hours
> (32768 tries), which would make the user of such a system much more
> likely to notice such an attack.
>
> I understand the desire for this clarification, and will happily try to
> improve the explanation for this change, especially so that those
> considering use of this option understand the tradeoffs, but I also view
> this as one particular hardening change which is a component of making
> attacks such as these harder, rather than the only solution.  As for the
> clarification itself, where would you like it?  I could include a cover
> letter for this patch-set, elaborate more in the commit message itself,
> add more to the Kconfig help description, or some combination of the above.

Unless I am mistaken this there is no cross over between different
processes of this randomization.  Would it make sense to have this as
an rlimit so that if you have processes on the system that are affected
by the tradeoff differently this setting can be changed per process?

Eric

Re: [PATCH v2 1/2] mm: mmap: Add new /proc tunable for mmap_base ASLR.

[Kees Cook]

On Wed, Nov 4, 2015 at 2:10 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Daniel Cashman <dcashman@android.com> writes:
>
>> On 11/3/15 5:31 PM, Andrew Morton wrote:
>>> On Tue, 03 Nov 2015 18:40:31 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:
>>>
>>>> Andrew Morton <akpm@linux-foundation.org> writes:
>>>>
>>>>> On Tue,  3 Nov 2015 10:10:03 -0800 Daniel Cashman <dcashman@android.com> wrote:
>>>>>
>>>>>> ASLR currently only uses 8 bits to generate the random offset for the
>>>>>> mmap base address on 32 bit architectures. This value was chosen to
>>>>>> prevent a poorly chosen value from dividing the address space in such
>>>>>> a way as to prevent large allocations. This may not be an issue on all
>>>>>> platforms. Allow the specification of a minimum number of bits so that
>>>>>> platforms desiring greater ASLR protection may determine where to place
>>>>>> the trade-off.
>>>>>
>>>>> Can we please include a very good description of the motivation for this
>>>>> change?  What is inadequate about the current code, what value does the
>>>>> enhancement have to our users, what real-world problems are being solved,
>>>>> etc.
>>>>>
>>>>> Because all we have at present is "greater ASLR protection", which doesn't
>>>>> really tell anyone anything.
>>>>
>>>> The description seemed clear to me.
>>>>
>>>> More random bits, more entropy, more work needed to brute force.
>>>>
>>>> 8 bits only requires 256 tries (or a 1 in 256) chance to brute force
>>>> something.
>>>
>>> Of course, but that's not really very useful.
>>>
>>>> We have seen in the last couple of months on Android how only having 8 bits
>>>> doesn't help much.
>>>
>>> Now THAT is important.  What happened here and how well does the
>>> proposed fix improve things?  How much longer will a brute-force attack
>>> take to succeed, with a particular set of kernel parameters?  Is the
>>> new duration considered to be sufficiently long and if not, are there
>>> alternative fixes we should be looking at?
>>>
>>> Stuff like this.
>>>
>>>> Each additional bit doubles the protection (and unfortunately also
>>>> increases fragmentation of the userspace address space).
>>>
>>> OK, so the benefit comes with a cost and people who are configuring
>>> systems (and the people who are reviewing this patchset!) need to
>>> understand the tradeoffs.  Please.
>>
>> The direct motivation here was in response to the libstagefright
>> vulnerabilities that affected Android, specifically to information
>> provided by Google's project zero at:
>>
>> http://googleprojectzero.blogspot.com/2015/09/stagefrightened.html
>>
>> The attack there specifically used the limited randomness used in
>> generating the mmap base address as part of a brute-force-based exploit.
>>  In this particular case, the attack was against the mediaserver process
>> on Android, which was limited to respawning every 5 seconds, giving the
>> attacker an average expected success rate of defeating the mmap ASLR
>> after over 10 minutes (128 tries at 5 seconds each).  With change to the
>> maximum proposed value of 16 bits, this would change to over 45 hours
>> (32768 tries), which would make the user of such a system much more
>> likely to notice such an attack.
>>
>> I understand the desire for this clarification, and will happily try to
>> improve the explanation for this change, especially so that those
>> considering use of this option understand the tradeoffs, but I also view
>> this as one particular hardening change which is a component of making
>> attacks such as these harder, rather than the only solution.  As for the
>> clarification itself, where would you like it?  I could include a cover
>> letter for this patch-set, elaborate more in the commit message itself,
>> add more to the Kconfig help description, or some combination of the above.
>
> Unless I am mistaken this there is no cross over between different
> processes of this randomization.  Would it make sense to have this as
> an rlimit so that if you have processes on the system that are affected
> by the tradeoff differently this setting can be changed per process?

I think that could be a good future bit of work, but I'd want to get
this in for all architectures first, so we have a more common base to
work from before introducing a new rlimit.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Daniel Cashman]

On 11/04/2015 10:30 AM, Daniel Cashman wrote:
> On 11/3/15 3:21 PM, Kees Cook wrote:
>> On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
>>> On 11/03/2015 11:19 AM, Kees Cook wrote:
>>>> Do you have patches for x86 and arm64?
>>>
>>> I was holding off on those until I could gauge upstream reception.  If
>>> desired, I could put those together and add them as [PATCH 3/4] and
>>> [PATCH 4/4].
>>
>> If they're as trivial as I'm hoping, yeah, let's toss them in now. If
>> not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
>> too, but one or two of those have somewhat stranger calculations when
>> I looked, so their Kconfigs may not be as clean.
> 
> Creating the patches should be simple, it's the choice of minimum and
> maximum values for each architecture that I'd be most concerned about.
> I'll put them together, though, and the ranges can be changed following
> discussion with those more knowledgeable, if needed.  I also don't have
> devices on which to test the PowerPC, MIPS and s390 changes, so I'll
> need someone's help for that.

Actually, in preparing the x86 and arm64 patches, it became apparent
that the current patch-set does not address 32-bit executables running
on 64-bit systems (compatibility mode), since only one procfs
mmap_rnd_bits variable is created and exported. Some possible solutions:

1) Create a second set for compatibility, e.g. mmap_rnd_compat_bits,
mmap_rnd_compat_bits_min, mmap_rnd_compat_bits_max and export it as with
mmap_rnd_bits.  This provides the most control and is truest to the
spirit of this patch, but pollutes the Kconfigs and procfs a bit more,
especially if we ever need a mmap_rnd_64compat_bits...

2) Get rid of the arch-independent nature of this patch and instead let
each arch define its own Kconfig values and procfs entries. Essentially
the same outcome as the above, but with less disruption in the common
kernel code, although also with a potentially variable ABI.

3) Default to the lowest-supported, e.g. arm64 running with
CONFIG_COMPAT would be limited to the same range as arm.  This solution
I think is highly undesirable, as it actually makes things worse for
existing 64-bit platforms.

4) Support setting the COMPAT values by Kconfig, but don't expose them
via procfs.  This keeps the procfs change simple and gets most of its
benefits.

5) Leave the COMPAT values specified in code, and only adjust introduce
config and tunable options for the 64-bit processes.  Basically keep
this patch-set as-is and not give any benefit to compatible applications.

My preference would be for either solutions 1 or 4, but would love
feedback and/or other solutions. Thoughts?

Thank You,
Dan

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Kees Cook]

On Thu, Nov 5, 2015 at 10:44 AM, Daniel Cashman <dcashman@android.com> wrote:
> On 11/04/2015 10:30 AM, Daniel Cashman wrote:
>> On 11/3/15 3:21 PM, Kees Cook wrote:
>>> On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
>>>> On 11/03/2015 11:19 AM, Kees Cook wrote:
>>>>> Do you have patches for x86 and arm64?
>>>>
>>>> I was holding off on those until I could gauge upstream reception.  If
>>>> desired, I could put those together and add them as [PATCH 3/4] and
>>>> [PATCH 4/4].
>>>
>>> If they're as trivial as I'm hoping, yeah, let's toss them in now. If
>>> not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
>>> too, but one or two of those have somewhat stranger calculations when
>>> I looked, so their Kconfigs may not be as clean.
>>
>> Creating the patches should be simple, it's the choice of minimum and
>> maximum values for each architecture that I'd be most concerned about.
>> I'll put them together, though, and the ranges can be changed following
>> discussion with those more knowledgeable, if needed.  I also don't have
>> devices on which to test the PowerPC, MIPS and s390 changes, so I'll
>> need someone's help for that.
>
> Actually, in preparing the x86 and arm64 patches, it became apparent
> that the current patch-set does not address 32-bit executables running
> on 64-bit systems (compatibility mode), since only one procfs
> mmap_rnd_bits variable is created and exported. Some possible solutions:
>
> 1) Create a second set for compatibility, e.g. mmap_rnd_compat_bits,
> mmap_rnd_compat_bits_min, mmap_rnd_compat_bits_max and export it as with
> mmap_rnd_bits.  This provides the most control and is truest to the
> spirit of this patch, but pollutes the Kconfigs and procfs a bit more,
> especially if we ever need a mmap_rnd_64compat_bits...
>
> 2) Get rid of the arch-independent nature of this patch and instead let
> each arch define its own Kconfig values and procfs entries. Essentially
> the same outcome as the above, but with less disruption in the common
> kernel code, although also with a potentially variable ABI.
>
> 3) Default to the lowest-supported, e.g. arm64 running with
> CONFIG_COMPAT would be limited to the same range as arm.  This solution
> I think is highly undesirable, as it actually makes things worse for
> existing 64-bit platforms.
>
> 4) Support setting the COMPAT values by Kconfig, but don't expose them
> via procfs.  This keeps the procfs change simple and gets most of its
> benefits.
>
> 5) Leave the COMPAT values specified in code, and only adjust introduce
> config and tunable options for the 64-bit processes.  Basically keep
> this patch-set as-is and not give any benefit to compatible applications.
>
> My preference would be for either solutions 1 or 4, but would love
> feedback and/or other solutions. Thoughts?

How about a single new CONFIG+sysctl that is the compat delta. For
example, on x86, it's 20 bits. Then we don't get splashed with a whole
new set of min/maxes, but we can reasonably control compat?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Michael Ellerman]

On Fri, 2015-11-06 at 12:52 -0800, Kees Cook wrote:
> On Thu, Nov 5, 2015 at 10:44 AM, Daniel Cashman <dcashman@android.com> wrote:
> > On 11/04/2015 10:30 AM, Daniel Cashman wrote:
> > > On 11/3/15 3:21 PM, Kees Cook wrote:
> > > > On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
> > > > > On 11/03/2015 11:19 AM, Kees Cook wrote:
> > > > > > Do you have patches for x86 and arm64?
> > > > > 
> > > > > I was holding off on those until I could gauge upstream reception.  If
> > > > > desired, I could put those together and add them as [PATCH 3/4] and
> > > > > [PATCH 4/4].
> > > > 
> > > > If they're as trivial as I'm hoping, yeah, let's toss them in now. If
> > > > not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
> > > > too, but one or two of those have somewhat stranger calculations when
> > > > I looked, so their Kconfigs may not be as clean.
> > > 
> > > Creating the patches should be simple, it's the choice of minimum and
> > > maximum values for each architecture that I'd be most concerned about.
> > > I'll put them together, though, and the ranges can be changed following
> > > discussion with those more knowledgeable, if needed.  I also don't have
> > > devices on which to test the PowerPC, MIPS and s390 changes, so I'll
> > > need someone's help for that.
> > 
> > Actually, in preparing the x86 and arm64 patches, it became apparent
> > that the current patch-set does not address 32-bit executables running
> > on 64-bit systems (compatibility mode), since only one procfs
> > mmap_rnd_bits variable is created and exported. Some possible solutions:
> 
> How about a single new CONFIG+sysctl that is the compat delta. For
> example, on x86, it's 20 bits. Then we don't get splashed with a whole
> new set of min/maxes, but we can reasonably control compat?

Do you mean in addition to mmap_rnd_bits?

So we'd end up with mmap_rnd_bits and also mmap_rnd_bits_compat_delta?
(naming TBD)

If so yeah I think that would work.

It would have the nice property of allowing you to add some more randomness to
all processes by bumping mmap_rnd_bits. But at the same time if you want to add
a lot more randomness to 64-bit processes, but just a bit (or none) to 32-bit
processes you can also do that.

cheers

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Daniel Cashman]

On 11/08/2015 07:47 PM, Michael Ellerman wrote:
> On Fri, 2015-11-06 at 12:52 -0800, Kees Cook wrote:
>> On Thu, Nov 5, 2015 at 10:44 AM, Daniel Cashman <dcashman@android.com> wrote:
>>> On 11/04/2015 10:30 AM, Daniel Cashman wrote:
>>>> On 11/3/15 3:21 PM, Kees Cook wrote:
>>>>> On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
>>>>>> On 11/03/2015 11:19 AM, Kees Cook wrote:
>>>>>>> Do you have patches for x86 and arm64?
>>>>>>
>>>>>> I was holding off on those until I could gauge upstream reception.  If
>>>>>> desired, I could put those together and add them as [PATCH 3/4] and
>>>>>> [PATCH 4/4].
>>>>>
>>>>> If they're as trivial as I'm hoping, yeah, let's toss them in now. If
>>>>> not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
>>>>> too, but one or two of those have somewhat stranger calculations when
>>>>> I looked, so their Kconfigs may not be as clean.
>>>>
>>>> Creating the patches should be simple, it's the choice of minimum and
>>>> maximum values for each architecture that I'd be most concerned about.
>>>> I'll put them together, though, and the ranges can be changed following
>>>> discussion with those more knowledgeable, if needed.  I also don't have
>>>> devices on which to test the PowerPC, MIPS and s390 changes, so I'll
>>>> need someone's help for that.
>>>
>>> Actually, in preparing the x86 and arm64 patches, it became apparent
>>> that the current patch-set does not address 32-bit executables running
>>> on 64-bit systems (compatibility mode), since only one procfs
>>> mmap_rnd_bits variable is created and exported. Some possible solutions:
>>
>> How about a single new CONFIG+sysctl that is the compat delta. For
>> example, on x86, it's 20 bits. Then we don't get splashed with a whole
>> new set of min/maxes, but we can reasonably control compat?
> 
> Do you mean in addition to mmap_rnd_bits?
> 
> So we'd end up with mmap_rnd_bits and also mmap_rnd_bits_compat_delta?
> (naming TBD)
> 
> If so yeah I think that would work.
> 
> It would have the nice property of allowing you to add some more randomness to
> all processes by bumping mmap_rnd_bits. But at the same time if you want to add
> a lot more randomness to 64-bit processes, but just a bit (or none) to 32-bit
> processes you can also do that.

I may be misunderstanding the suggestion, or perhaps simply too
conservative in my desire to prevent bad values, but I still think we
would have need for two min-max ranges.  If using a single
mmap_rnd_bits_compat value, there are two approaches: to either use
mmap_rnd_bits for 32-bit applications and then add the compat value for
64-bit or the opposite, to have mmap_rnd_bits be the default and
subtract the compat value for the 32-bit applications.  In either case,
the compat value would need to be sensibly bounded, and that bounding
depends on acceptable values for both 32 and 64 bit applications.

Re: [PATCH v2 2/2] arm: mm: support ARCH_MMAP_RND_BITS.

[Kees Cook]

On Mon, Nov 9, 2015 at 10:56 AM, Daniel Cashman <dcashman@android.com> wrote:
> On 11/08/2015 07:47 PM, Michael Ellerman wrote:
>> On Fri, 2015-11-06 at 12:52 -0800, Kees Cook wrote:
>>> On Thu, Nov 5, 2015 at 10:44 AM, Daniel Cashman <dcashman@android.com> wrote:
>>>> On 11/04/2015 10:30 AM, Daniel Cashman wrote:
>>>>> On 11/3/15 3:21 PM, Kees Cook wrote:
>>>>>> On Tue, Nov 3, 2015 at 3:14 PM, Daniel Cashman <dcashman@android.com> wrote:
>>>>>>> On 11/03/2015 11:19 AM, Kees Cook wrote:
>>>>>>>> Do you have patches for x86 and arm64?
>>>>>>>
>>>>>>> I was holding off on those until I could gauge upstream reception.  If
>>>>>>> desired, I could put those together and add them as [PATCH 3/4] and
>>>>>>> [PATCH 4/4].
>>>>>>
>>>>>> If they're as trivial as I'm hoping, yeah, let's toss them in now. If
>>>>>> not, skip 'em. PowerPC, MIPS, and s390 should be relatively simple
>>>>>> too, but one or two of those have somewhat stranger calculations when
>>>>>> I looked, so their Kconfigs may not be as clean.
>>>>>
>>>>> Creating the patches should be simple, it's the choice of minimum and
>>>>> maximum values for each architecture that I'd be most concerned about.
>>>>> I'll put them together, though, and the ranges can be changed following
>>>>> discussion with those more knowledgeable, if needed.  I also don't have
>>>>> devices on which to test the PowerPC, MIPS and s390 changes, so I'll
>>>>> need someone's help for that.
>>>>
>>>> Actually, in preparing the x86 and arm64 patches, it became apparent
>>>> that the current patch-set does not address 32-bit executables running
>>>> on 64-bit systems (compatibility mode), since only one procfs
>>>> mmap_rnd_bits variable is created and exported. Some possible solutions:
>>>
>>> How about a single new CONFIG+sysctl that is the compat delta. For
>>> example, on x86, it's 20 bits. Then we don't get splashed with a whole
>>> new set of min/maxes, but we can reasonably control compat?
>>
>> Do you mean in addition to mmap_rnd_bits?
>>
>> So we'd end up with mmap_rnd_bits and also mmap_rnd_bits_compat_delta?
>> (naming TBD)
>>
>> If so yeah I think that would work.
>>
>> It would have the nice property of allowing you to add some more randomness to
>> all processes by bumping mmap_rnd_bits. But at the same time if you want to add
>> a lot more randomness to 64-bit processes, but just a bit (or none) to 32-bit
>> processes you can also do that.
>
> I may be misunderstanding the suggestion, or perhaps simply too
> conservative in my desire to prevent bad values, but I still think we
> would have need for two min-max ranges.  If using a single
> mmap_rnd_bits_compat value, there are two approaches: to either use
> mmap_rnd_bits for 32-bit applications and then add the compat value for
> 64-bit or the opposite, to have mmap_rnd_bits be the default and
> subtract the compat value for the 32-bit applications.  In either case,
> the compat value would need to be sensibly bounded, and that bounding
> depends on acceptable values for both 32 and 64 bit applications.

Yeah, I think I wasn't thinking about this well. I think two sysctls
should be fine: mmap_rnd_bits and mmap_compat_rnd_bits. And
internally, we'd have a second set of CONFIGs (and ranges) to deal
with CONFIG_COMPAT.

-Kees

-- 
Kees Cook
Chrome OS Security