subject:"mm\: insure topdown mmap chooses addresses above security minimum"

Re: mm: insure topdown mmap chooses addresses above security minimum

2013-09-27 Thread Timothy Pepper

On Wed 25 Sep at 19:44:36 +0200 mi...@kernel.org said:
 
 * Timothy Pepper timothy.c.pep...@linux.intel.com wrote:
 
  On Wed 25 Sep at 09:30:49 +0200 mi...@kernel.org said:
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 timothy.c.pep...@linux.intel.com
Intel Open Source Technology Center
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: mm: insure topdown mmap chooses addresses above security minimum

2013-09-25 Thread Ingo Molnar


* Timothy Pepper timothy.c.pep...@linux.intel.com 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 timothy.c.pep...@linux.intel.com
 Cc: linux...@kvack.org
 Cc: Thomas Gleixner t...@linutronix.de
 Cc: Ingo Molnar mi...@redhat.com
 Cc: H. Peter Anvin h...@zytor.com
 Cc: x...@kernel.org
 Cc: Russell King li...@arm.linux.org.uk
 Cc: linux-arm-ker...@lists.infradead.org
 Cc: Ralf Baechle r...@linux-mips.org
 Cc: linux-m...@linux-mips.org
 Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
 Cc: Paul Mackerras pau...@samba.org
 Cc: linuxppc-dev@lists.ozlabs.org
 Cc: Paul Mundt let...@linux-sh.org
 Cc: linux...@vger.kernel.org
 Cc: David S. Miller da...@davemloft.net
 Cc: sparcli...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: mm: insure topdown mmap chooses addresses above security minimum

2013-09-25 Thread Timothy Pepper

On Wed 25 Sep at 09:30:49 +0200 mi...@kernel.org said:
  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 timothy.c.pep...@linux.intel.com
Intel Open Source Technology Center
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: mm: insure topdown mmap chooses addresses above security minimum

2013-09-25 Thread Ingo Molnar


* Timothy Pepper timothy.c.pep...@linux.intel.com wrote:

 On Wed 25 Sep at 09:30:49 +0200 mi...@kernel.org said:
 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: mm: insure topdown mmap chooses addresses above security minimum

2013-09-24 Thread Russell King - ARM Linux

On Tue, Sep 24, 2013 at 02:23:31PM -0700, Timothy Pepper wrote:
 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;

This looks sane for ARM.

Acked-by: Russell King rmk+ker...@arm.linux.org.uk

Thanks.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

mm: insure topdown mmap chooses addresses above security minimum

2013-09-24 Thread Timothy Pepper

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 timothy.c.pep...@linux.intel.com
Cc: linux...@kvack.org
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: x...@kernel.org
Cc: Russell King li...@arm.linux.org.uk
Cc: linux-arm-ker...@lists.infradead.org
Cc: Ralf Baechle r...@linux-mips.org
Cc: linux-m...@linux-mips.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Paul Mundt let...@linux-sh.org
Cc: linux...@vger.kernel.org
Cc: David S. Miller da...@davemloft.net
Cc: sparcli...@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

Re: mm: insure topdown mmap chooses addresses above security minimum

Re: mm: insure topdown mmap chooses addresses above security minimum

Re: mm: insure topdown mmap chooses addresses above security minimum

Re: mm: insure topdown mmap chooses addresses above security minimum

Re: mm: insure topdown mmap chooses addresses above security minimum

mm: insure topdown mmap chooses addresses above security minimum

6 matches

Site Navigation

Mail list logo

Footer information