mm: insure topdown mmap chooses addresses above security minimum

A security check is performed on mmap addresses in
security/security.c:security_mmap_addr().  It uses mmap_min_addr to insure
mmaps don't get addresses lower than a user configurable guard value
(/proc/sys/vm/mmap_min_addr).  The arch specific mmap topdown searches
look for a map candidate address all the way down to a low_limit that is
currently hard coded as PAGE_SIZE.  Depending on compile time options
and userspace setting the procfs tunable, the security check's view of
the minimum allowable address may be something greater than PAGE_SIZE.
This leaves a gap where get_unmapped_area()'s call to get_area() might
return an address above PAGE_SIZE, but below mmap_min_addr, and thus
get_unmapped_area() fails.

This was seen on x86_64 in the case of a topdown address space and a large
stack rlimit, with mmap_min_addr having been set to 32k by the distro.
This left a 28k gap where the get area search intends to place a small
mmap, but then get_unmapped_area() stumbles at the security check.

What should have happened is the address search wraps back to a higher
address, the search continues and perhaps succeeds.  Indeed an mmap of
a larger size gets a topdown search that does wrap around back up into
the rlimit stack reserve and succeeds assuming suitable free space.
But a small mmap fits in the low gap and always fails.  It becomes
possible to make large mmaps but not small ones.

When an explicit address hint is given, mm/mmap.c's round_hint_to_min()
will round up to mmap_min_addr.

A topdown search's low_limit should similarly consider mmap_min_addr
instead of just PAGE_SIZE.

Signed-off-by: Tim Pepper <redacted>
Cc: linux-mm@kvack.org
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Russell King <redacted>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Ralf Baechle <redacted>
Cc: linux-mips@linux-mips.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Paul Mundt <redacted>
Cc: linux-sh@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
--
 arch/arm/mm/mmap.c               | 3 ++-
 arch/mips/mm/mmap.c              | 3 ++-
 arch/powerpc/mm/slice.c          | 3 ++-
 arch/sh/mm/mmap.c                | 3 ++-
 arch/sparc/kernel/sys_sparc_64.c | 3 ++-
 arch/x86/kernel/sys_x86_64.c     | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index 0c63562..0e7355d 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c

@@ -9,6 +9,7 @@
 #include <linux/io.h>
 #include <linux/personality.h>
 #include <linux/random.h>
+#include <linux/security.h>
 #include <asm/cachetype.h>
 
 #define COLOUR_ALIGN(addr,pgoff)		\

@@ -146,7 +147,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index f1baadd..8c0deb7 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c

@@ -14,6 +14,7 @@
 #include <linux/personality.h>
 #include <linux/random.h>
 #include <linux/sched.h>
+#include <linux/security.h>
 
 unsigned long shm_align_mask = PAGE_SIZE - 1;	/* Sane caches */
 EXPORT_SYMBOL(shm_align_mask);

@@ -102,7 +103,7 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
 
 	if (dir == DOWN) {
 		info.flags = VM_UNMAPPED_AREA_TOPDOWN;
-		info.low_limit = PAGE_SIZE;
+		info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 		info.high_limit = mm->mmap_base;
 		addr = vm_unmapped_area(&info);
 

diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index 3e99c14..34fc601 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c

@@ -30,6 +30,7 @@
 #include <linux/err.h>
 #include <linux/spinlock.h>
 #include <linux/export.h>
+#include <linux/security.h>
 #include <asm/mman.h>
 #include <asm/mmu.h>
 #include <asm/spu.h>

@@ -338,7 +339,7 @@ static unsigned long slice_find_area_topdown(struct mm_struct *mm,
 			addr = prev;
 			goto prev_slice;
 		}
-		info.low_limit = addr;
+		info.low_limit = max(addr, PAGE_ALIGN(mmap_min_addr));
 
 		found = vm_unmapped_area(&info);
 		if (!(found & ~PAGE_MASK))

diff --git a/arch/sh/mm/mmap.c b/arch/sh/mm/mmap.c
index 6777177..1e0c53d 100644
--- a/arch/sh/mm/mmap.c
+++ b/arch/sh/mm/mmap.c

@@ -11,6 +11,7 @@
 #include <linux/mm.h>
 #include <linux/mman.h>
 #include <linux/module.h>
+#include <linux/security.h>
 #include <asm/page.h>
 #include <asm/processor.h>
 

@@ -119,7 +120,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = do_colour_align ? (PAGE_MASK & shm_align_mask) : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c
index 51561b8..dab0a5d 100644
--- a/arch/sparc/kernel/sys_sparc_64.c
+++ b/arch/sparc/kernel/sys_sparc_64.c

@@ -24,6 +24,7 @@
 #include <linux/personality.h>
 #include <linux/random.h>
 #include <linux/export.h>
+#include <linux/security.h>
 
 #include <asm/uaccess.h>
 #include <asm/utrap.h>

@@ -188,7 +189,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = do_color_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index 30277e2..93e563e 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c

@@ -15,6 +15,7 @@
 #include <linux/random.h>
 #include <linux/uaccess.h>
 #include <linux/elf.h>
+#include <linux/security.h>
 
 #include <asm/ia32.h>
 #include <asm/syscalls.h>

@@ -172,7 +173,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = filp ? get_align_mask() : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

On Tue, Sep 24, 2013 at 02:23:31PM -0700, Timothy Pepper wrote:

This looks sane for ARM.

Acked-by: Russell King <redacted>

Thanks.

* Timothy Pepper [off-list ref] wrote:

A security check is performed on mmap addresses in
security/security.c:security_mmap_addr().  It uses mmap_min_addr to insure
mmaps don't get addresses lower than a user configurable guard value
(/proc/sys/vm/mmap_min_addr).  The arch specific mmap topdown searches
look for a map candidate address all the way down to a low_limit that is
currently hard coded as PAGE_SIZE.  Depending on compile time options
and userspace setting the procfs tunable, the security check's view of
the minimum allowable address may be something greater than PAGE_SIZE.
This leaves a gap where get_unmapped_area()'s call to get_area() might
return an address above PAGE_SIZE, but below mmap_min_addr, and thus
get_unmapped_area() fails.

This was seen on x86_64 in the case of a topdown address space and a large
stack rlimit, with mmap_min_addr having been set to 32k by the distro.
This left a 28k gap where the get area search intends to place a small
mmap, but then get_unmapped_area() stumbles at the security check.

What should have happened is the address search wraps back to a higher
address, the search continues and perhaps succeeds.  Indeed an mmap of
a larger size gets a topdown search that does wrap around back up into
the rlimit stack reserve and succeeds assuming suitable free space.
But a small mmap fits in the low gap and always fails.  It becomes
possible to make large mmaps but not small ones.

When an explicit address hint is given, mm/mmap.c's round_hint_to_min()
will round up to mmap_min_addr.

A topdown search's low_limit should similarly consider mmap_min_addr
instead of just PAGE_SIZE.

Signed-off-by: Tim Pepper <redacted>
Cc: linux-mm@kvack.org
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Russell King <redacted>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Ralf Baechle <redacted>
Cc: linux-mips@linux-mips.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Paul Mundt <redacted>
Cc: linux-sh@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
--
 arch/arm/mm/mmap.c               | 3 ++-
 arch/mips/mm/mmap.c              | 3 ++-
 arch/powerpc/mm/slice.c          | 3 ++-
 arch/sh/mm/mmap.c                | 3 ++-
 arch/sparc/kernel/sys_sparc_64.c | 3 ++-
 arch/x86/kernel/sys_x86_64.c     | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

 		info.flags = VM_UNMAPPED_AREA_TOPDOWN;
-		info.low_limit = PAGE_SIZE;
+		info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 		info.high_limit = mm->mmap_base;
 		addr = vm_unmapped_area(&info);

-		info.low_limit = addr;
+		info.low_limit = max(addr, PAGE_ALIGN(mmap_min_addr));

 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = do_colour_align ? (PAGE_MASK & shm_align_mask) : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = do_color_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = filp ? get_align_mask() : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

There appears to be a lot of repetition in these methods - instead of 
changing 6 places it would be more future-proof to first factor out the 
common bits and then to apply the fix to the shared implementation.

Thanks,

	Ingo

On Wed 25 Sep at 09:30:49 +0200 mingo@kernel.org said:

quoted

 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = filp ? get_align_mask() : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

There appears to be a lot of repetition in these methods - instead of 
changing 6 places it would be more future-proof to first factor out the 
common bits and then to apply the fix to the shared implementation.

Besides that existing redundancy in the multiple somewhat similar
arch_get_unmapped_area_topdown() functions, I was expecting people might
question the added redundancy of the six instances of:

	max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));

There's also a seventh similar instance if you consider
mm/mmap.c:round_hint_to_min() and its call stack.  I'm inclined to
think mmap_min_addr should be validated/aligned in one place, namely on
initialization and input in security/min_addr.c:update_mmap_min_addr(),
with mmap_min_addr always stored as an aligned value.

In the past commit 40401530 Al Viro arguably moved that checking out
of the security code and toward the mmap code.  Granted at that point
though there was only the round_hint_to_min() insuring the value in
mmap_min_addr was page aligned before use in that call path.  I'm thinking
something like:

diff --git a/security/min_addr.c b/security/min_addr.c
--- a/security/min_addr.c
+++ b/security/min_addr.c

@@ -14,14 +14,16 @@ unsigned long dac_mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR;
  */
 static void update_mmap_min_addr(void)
 {
+	unsigned long addr;
 #ifdef CONFIG_LSM_MMAP_MIN_ADDR
 	if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR)
-		mmap_min_addr = dac_mmap_min_addr;
+		addr = dac_mmap_min_addr;
 	else
-		mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
+		addr = CONFIG_LSM_MMAP_MIN_ADDR;
 #else
-	mmap_min_addr = dac_mmap_min_addr;
+	addr = dac_mmap_min_addr;
 #endif
+	mmap_min_addr = max(PAGE_SIZE, PAGE_ALIGN(addr));
 }
 
 /*

But this possibly has implications beyond the mmap code.

Al Viro, James Morris: any thoughts on the above?

Michel, Rik: what do you think of common helpers called by
ARM, MIPS, SH, Sparc, x86_64 arch_get_unmapped_area_topdown()
and arch_get_unmapped_area() to handle initialization of struct
vm_unmapped_area_info info fields which are currently mostly common?
Given the nuances of "mostly common" I'm not sure the result would
actually be positive for overall readability / self-documenting-ness of
the per arch files.

-- 
Tim Pepper [off-list ref]
Intel Open Source Technology Center

* Timothy Pepper [off-list ref] wrote:

On Wed 25 Sep at 09:30:49 +0200 mingo@kernel.org said:

quoted

quoted

 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = filp ? get_align_mask() : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

There appears to be a lot of repetition in these methods - instead of 
changing 6 places it would be more future-proof to first factor out the 
common bits and then to apply the fix to the shared implementation.

Besides that existing redundancy in the multiple somewhat similar
arch_get_unmapped_area_topdown() functions, I was expecting people might
question the added redundancy of the six instances of:

	max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));

That redundancy would be automatically addressed by my suggestion.

Thanks,

	Ingo

On Wed 25 Sep at 19:44:36 +0200 mingo@kernel.org said:

* Timothy Pepper [off-list ref] wrote:

quoted

On Wed 25 Sep at 09:30:49 +0200 mingo@kernel.org said:

quoted

quoted

 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
-	info.low_limit = PAGE_SIZE;
+	info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
 	info.high_limit = mm->mmap_base;
 	info.align_mask = filp ? get_align_mask() : 0;
 	info.align_offset = pgoff << PAGE_SHIFT;

There appears to be a lot of repetition in these methods - instead of 
changing 6 places it would be more future-proof to first factor out the 
common bits and then to apply the fix to the shared implementation.

Besides that existing redundancy in the multiple somewhat similar
arch_get_unmapped_area_topdown() functions, I was expecting people might
question the added redundancy of the six instances of:

	max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));

That redundancy would be automatically addressed by my suggestion.

Yes.

I'm looking at the cleanup and will post a bisectable series that
introduces a common helper, addes the calls to use that helper where
applicable (looks like it might be a few dozen per arch locations), and
then the single line change for the topdown case within the common helper
to do:

	info->low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));

-- 
Tim Pepper [off-list ref]
Intel Open Source Technology Center