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

[PATCH 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>
---
 Documentation/sysctl/kernel.txt | 14 ++++++++++++++
 include/linux/mm.h              |  6 ++++++
 kernel/sysctl.c                 | 11 +++++++++++
 3 files changed, 31 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/include/linux/mm.h b/include/linux/mm.h
index 80001de..15b083a 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_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..37e657a 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_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
 	{ }
 };
 
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 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>
---
 arch/arm/Kconfig   | 24 ++++++++++++++++++++++++
 arch/arm/mm/mmap.c |  7 +++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 639411f..d61e7e2 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -306,6 +306,30 @@ 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
+	int
+	default 8
+
+config ARCH_MMAP_RND_BITS_MAX
+	int
+	default 14 if MMU && PAGE_OFFSET=0x40000000
+	default 15 if MMU && PAGE_OFFSET=0x80000000
+	default 16 if MMU
+	default 8
+
+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
+	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
+
 #
 # 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..73ca3a7 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -11,6 +11,10 @@
 #include <linux/random.h>
 #include <asm/cachetype.h>
 
+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;
+
 #define COLOUR_ALIGN(addr,pgoff)		\
 	((((addr)+SHMLBA-1)&~(SHMLBA-1)) +	\
 	 (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
@@ -173,8 +177,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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Eric W. Biederman]

Daniel Cashman <dcashman@android.com> writes:

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

This all would be much cleaner if the arm architecture code were just to
register the sysctl itself.

As it sits this looks like a patchset that does not meaninfully bisect,
and would result in code that is hard to trace and understand.

Eric

> Signed-off-by: Daniel Cashman <dcashman@google.com>
> ---
>  Documentation/sysctl/kernel.txt | 14 ++++++++++++++
>  include/linux/mm.h              |  6 ++++++
>  kernel/sysctl.c                 | 11 +++++++++++
>  3 files changed, 31 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/include/linux/mm.h b/include/linux/mm.h
> index 80001de..15b083a 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_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..37e657a 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_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
>  	{ }
>  };

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

>
> As it sits this looks like a patchset that does not meaninfully bisect,
> and would result in code that is hard to trace and understand.
>

I believe the intent is to follow up with more architecture specific
patches to allow each architecture to define the number of bits to use
(min, max, and default) since these values are architecture dependent.
Arm64 patch should be forthcoming, and others after that. With that in
mind, would you still prefer to have the sysctl code in the arm-specific
patch?

[-- Attachment #2: Type: text/html, Size: 700 bytes --]

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

[Jeffrey Vander Stoep]

plain text this time...

> This all would be much cleaner if the arm architecture code were just to
> register the sysctl itself.
>
> As it sits this looks like a patchset that does not meaninfully bisect,
> and would result in code that is hard to trace and understand.

I believe the intent is to follow up with more architecture specific
patches to allow each architecture to define the number of bits to use
(min, max, and default) since these values are architecture dependent.
Arm64 patch should be forthcoming, and others after that. With that in
mind, would you still prefer to have the sysctl code in the
arm-specific patch?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Dan Cashman]

> > This all would be much cleaner if the arm architecture code were just to
> > register the sysctl itself.
> >
> > As it sits this looks like a patchset that does not meaninfully bisect,
> > and would result in code that is hard to trace and understand.
>
> I believe the intent is to follow up with more architecture specific
> patches to allow each architecture to define the number of bits to use

Yes.  I included these patches together because they provide mutual
context, but each has a different outcome and they could be taken
separately.  The arm architecture-specific portion allows the changing
of the number of bits used for mmap ASLR, useful even without the
sysctl.  The sysctl patch (patch 1) provides another way of setting
this value, and the hope is that this will be adopted across multiple
architectures, with the arm changes (patch 2) providing an example.  I
hope to follow this with changes to arm64 and x86, for example.

On Wed, Oct 28, 2015 at 5:01 PM, Jeffrey Vander Stoep <jeffv@google.com> wrote:
> plain text this time...
>
>> This all would be much cleaner if the arm architecture code were just to
>> register the sysctl itself.
>>
>> As it sits this looks like a patchset that does not meaninfully bisect,
>> and would result in code that is hard to trace and understand.
>
> I believe the intent is to follow up with more architecture specific
> patches to allow each architecture to define the number of bits to use
> (min, max, and default) since these values are architecture dependent.
> Arm64 patch should be forthcoming, and others after that. With that in
> mind, would you still prefer to have the sysctl code in the
> arm-specific patch?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Eric W. Biederman]

Dan Cashman <dcashman@android.com> writes:

>> > This all would be much cleaner if the arm architecture code were just to
>> > register the sysctl itself.
>> >
>> > As it sits this looks like a patchset that does not meaninfully bisect,
>> > and would result in code that is hard to trace and understand.
>>
>> I believe the intent is to follow up with more architecture specific
>> patches to allow each architecture to define the number of bits to use
>
> Yes.  I included these patches together because they provide mutual
> context, but each has a different outcome and they could be taken
> separately.

They can not.  The first patch is incomplete by itself.

> The arm architecture-specific portion allows the changing
> of the number of bits used for mmap ASLR, useful even without the
> sysctl.  The sysctl patch (patch 1) provides another way of setting
> this value, and the hope is that this will be adopted across multiple
> architectures, with the arm changes (patch 2) providing an example.  I
> hope to follow this with changes to arm64 and x86, for example.

If you want to make the code generic.  Please maximize the sharing.
That is please define the variables in a generic location, as well
as the Kconfig variables (if possible).

As it is you have an architecture specific piece of code that can not be
reused without duplicating code, and that is just begging for problems.

Eric

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Daniel Cashman]

On 10/28/2015 08:41 PM, Eric W. Biederman wrote:
> Dan Cashman <dcashman@android.com> writes:
> 
>>>> This all would be much cleaner if the arm architecture code were just to
>>>> register the sysctl itself.
>>>>
>>>> As it sits this looks like a patchset that does not meaninfully bisect,
>>>> and would result in code that is hard to trace and understand.
>>>
>>> I believe the intent is to follow up with more architecture specific
>>> patches to allow each architecture to define the number of bits to use
>>
>> Yes.  I included these patches together because they provide mutual
>> context, but each has a different outcome and they could be taken
>> separately.
> 
> They can not.  The first patch is incomplete by itself.

Could you be more specific in what makes the first patch incomplete?  Is
it because it is essentially a no-op without additional architecture
changes (e.g. the second patch) or is it specifically because it
introduces and uses the three "mmap_rnd_bits*" variables without
defining them?  If the former, I'd like to avoid combining the general
procfs change with any architecture-specific one(s).  If the latter, I
hope the proposal below addresses that.

>> The arm architecture-specific portion allows the changing
>> of the number of bits used for mmap ASLR, useful even without the
>> sysctl.  The sysctl patch (patch 1) provides another way of setting
>> this value, and the hope is that this will be adopted across multiple
>> architectures, with the arm changes (patch 2) providing an example.  I
>> hope to follow this with changes to arm64 and x86, for example.
> 
> If you want to make the code generic.  Please maximize the sharing.
> That is please define the variables in a generic location, as well
> as the Kconfig variables (if possible).
> 
> As it is you have an architecture specific piece of code that can not be
> reused without duplicating code, and that is just begging for problems.

I think it would make sense to move the variable definitions into
mm/mmap.c, included conditionally based on the presence of
CONFIG_ARCH_MMAP_RND_BITS.

As for the Kconfigs, I am open to suggestions.  I considered declaring
and documenting ARCH_MMAP_RND_BITS in arch/Kconfig, but I would like it
to be bounded in range by the _MIN and _MAX values, which necessarily
must be defined in the arch-specific Kconfigs.  Thus, we'd have
ARCH_MMAP_RND_BITS declared in arch/Kconfig as it currently is in
arch/arm/Kconfig defaulting to _MIN, and would declare both the _MIN and
_MAX in arch/Kconfig, while specifying default values in
arch/${ARCH}/Kconfig.

Would these changes be more acceptable?

Thank You,
Dan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Eric W. Biederman]

Daniel Cashman <dcashman@android.com> writes:

> On 10/28/2015 08:41 PM, Eric W. Biederman wrote:
>> Dan Cashman <dcashman@android.com> writes:
>> 
>>>>> This all would be much cleaner if the arm architecture code were just to
>>>>> register the sysctl itself.
>>>>>
>>>>> As it sits this looks like a patchset that does not meaninfully bisect,
>>>>> and would result in code that is hard to trace and understand.
>>>>
>>>> I believe the intent is to follow up with more architecture specific
>>>> patches to allow each architecture to define the number of bits to use
>>>
>>> Yes.  I included these patches together because they provide mutual
>>> context, but each has a different outcome and they could be taken
>>> separately.
>> 
>> They can not.  The first patch is incomplete by itself.
>
> Could you be more specific in what makes the first patch incomplete?  Is
> it because it is essentially a no-op without additional architecture
> changes (e.g. the second patch) or is it specifically because it
> introduces and uses the three "mmap_rnd_bits*" variables without
> defining them?  If the former, I'd like to avoid combining the general
> procfs change with any architecture-specific one(s).  If the latter, I
> hope the proposal below addresses that.

A bit of both.  The fact that the code can not compile in the first
patch because of missing variables is distressing.  Having the arch
specific code as a separate patch is fine, but they need to remain in
the same patchset.

>>> The arm architecture-specific portion allows the changing
>>> of the number of bits used for mmap ASLR, useful even without the
>>> sysctl.  The sysctl patch (patch 1) provides another way of setting
>>> this value, and the hope is that this will be adopted across multiple
>>> architectures, with the arm changes (patch 2) providing an example.  I
>>> hope to follow this with changes to arm64 and x86, for example.
>> 
>> If you want to make the code generic.  Please maximize the sharing.
>> That is please define the variables in a generic location, as well
>> as the Kconfig variables (if possible).
>> 
>> As it is you have an architecture specific piece of code that can not be
>> reused without duplicating code, and that is just begging for problems.
>
> I think it would make sense to move the variable definitions into
> mm/mmap.c, included conditionally based on the presence of
> CONFIG_ARCH_MMAP_RND_BITS.
>
> As for the Kconfigs, I am open to suggestions.  I considered declaring
> and documenting ARCH_MMAP_RND_BITS in arch/Kconfig, but I would like it
> to be bounded in range by the _MIN and _MAX values, which necessarily
> must be defined in the arch-specific Kconfigs.  Thus, we'd have
> ARCH_MMAP_RND_BITS declared in arch/Kconfig as it currently is in
> arch/arm/Kconfig defaulting to _MIN, and would declare both the _MIN and
> _MAX in arch/Kconfig, while specifying default values in
> arch/${ARCH}/Kconfig.
>
> Would these changes be more acceptable?

Yes.  I don't think you can do much about the Kconfigs so I would not
worry about that too much.

Eric

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Daniel Cashman]

On 11/01/2015 01:50 PM, Eric W. Biederman wrote:
> Daniel Cashman <dcashman@android.com> writes:
> 
>> On 10/28/2015 08:41 PM, Eric W. Biederman wrote:
>>> Dan Cashman <dcashman@android.com> writes:
>>>
>>>>>> This all would be much cleaner if the arm architecture code were just to
>>>>>> register the sysctl itself.
>>>>>>
>>>>>> As it sits this looks like a patchset that does not meaninfully bisect,
>>>>>> and would result in code that is hard to trace and understand.
>>>>>
>>>>> I believe the intent is to follow up with more architecture specific
>>>>> patches to allow each architecture to define the number of bits to use
>>>>
>>>> Yes.  I included these patches together because they provide mutual
>>>> context, but each has a different outcome and they could be taken
>>>> separately.
>>>
>>> They can not.  The first patch is incomplete by itself.
>>
>> Could you be more specific in what makes the first patch incomplete?  Is
>> it because it is essentially a no-op without additional architecture
>> changes (e.g. the second patch) or is it specifically because it
>> introduces and uses the three "mmap_rnd_bits*" variables without
>> defining them?  If the former, I'd like to avoid combining the general
>> procfs change with any architecture-specific one(s).  If the latter, I
>> hope the proposal below addresses that.
> 
> A bit of both.  The fact that the code can not compile in the first
> patch because of missing variables is distressing.  Having the arch
> specific code as a separate patch is fine, but they need to remain in
> the same patchset.
> 

The first patch would compile as long as CONFIG_ARCH_MMAP_RND_BITS were
not defined without also defining the missing variables. I actually
viewed this as a safeguard against attempting to use those variables
without architecture support, but am ok with changing it.

I've gone ahead and submitted [PATCH v2] which aims to reduce
duplication in the arch-specific config files and concerning those
variables.  The only caveat is that now the second patch depends on the
first, whereas before it did not.

Thank You,
Dan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>