diff mbox

mm: insure topdown mmap chooses addresses above security minimum

Message ID 20130925171243.GA7428@tcpepper-desk.jf.intel.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Timothy Pepper Sept. 25, 2013, 5:12 p.m. UTC 
On Wed 25 Sep at 09:30:49 +0200 mingo@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:


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.

Comments

Ingo Molnar Sept. 25, 2013, 5:44 p.m. UTC | #1 
* Timothy Pepper <timothy.c.pepper@linux.intel.com> wrote:

> On Wed 25 Sep at 09:30:49 +0200 mingo@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
Timothy Pepper Sept. 27, 2013, 3:39 p.m. UTC | #2 
On Wed 25 Sep at 19:44:36 +0200 mingo@kernel.org said:
> 
> * Timothy Pepper <timothy.c.pepper@linux.intel.com> wrote:
> 
> > On Wed 25 Sep at 09:30:49 +0200 mingo@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));
diff mbox

Patch

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));
 }
 
 /*