Patchwork UBUNTU: SAUCE: x86: brk away from exec rand area

login
register
mail settings
Submitter Kees Cook
Date Jan. 15, 2010, 8:41 p.m.
Message ID <20100115204105.GB5185@outflux.net>
Download mbox | patch
Permalink /patch/42996/
State Changes Requested
Headers show

Comments

Kees Cook - Jan. 15, 2010, 8:41 p.m.
This is a fix for the NX emulation patch to force the brk area well outside
of the exec randomization area to avoid future allocation or brk growth
collisions.  Normally this isn't a problem, except when the text region
has been loaded from a PIE binary and the CS limit can't be put just above
bss.

Additionally, the nx-emulation patch was still randomizing addresses even when
randomize_va_space was disabled, which would cause collisions even faster if
someone tried to disable randomization.

BugLink: http://bugs.launchpad.net/bugs/452175

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 fs/binfmt_elf.c |   10 ++++++++++
 mm/mmap.c       |   11 ++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)
Jeremy Kerr - Jan. 16, 2010, 12:04 a.m.
Hi Kees,

> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index b10acea..73594b9 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -978,6 +978,16 @@ static int load_elf_binary(struct linux_binprm *bprm,
>  struct pt_regs *regs) #ifdef arch_randomize_brk
>  	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1))
>  		current->mm->brk = current->mm->start_brk =
> +# ifdef CONFIG_X86_32
> +			/* in the case of NX emulation, shove the brk
> +			   segment way out of the way of the exec
> +			   randomization area, since it can collide with
> +			   future allocations if not. */
> +			( (current->mm->get_unmapped_exec_area ==
> +			   arch_get_unmapped_exec_area) &&
> +			  (current->mm->brk < 0x08000000)
> +			  ? (TASK_SIZE/6) : 0) +
> +# endif
>  			arch_randomize_brk(current->mm);

Seeing as this is arch specific, it might be best to put it in 
arch_randomize_brk, if possible.

Cheers,


Jeremy
Kees Cook - Jan. 16, 2010, 12:18 a.m.
Hi Jeremy,

On Sat, Jan 16, 2010 at 11:04:05AM +1100, Jeremy Kerr wrote:
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index b10acea..73594b9 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -978,6 +978,16 @@ static int load_elf_binary(struct linux_binprm *bprm,
> >  struct pt_regs *regs) #ifdef arch_randomize_brk
> >  	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1))
> >  		current->mm->brk = current->mm->start_brk =
> > +# ifdef CONFIG_X86_32
> > +			/* in the case of NX emulation, shove the brk
> > +			   segment way out of the way of the exec
> > +			   randomization area, since it can collide with
> > +			   future allocations if not. */
> > +			( (current->mm->get_unmapped_exec_area ==
> > +			   arch_get_unmapped_exec_area) &&
> > +			  (current->mm->brk < 0x08000000)
> > +			  ? (TASK_SIZE/6) : 0) +
> > +# endif
> >  			arch_randomize_brk(current->mm);
> 
> Seeing as this is arch specific, it might be best to put it in 
> arch_randomize_brk, if possible.

arch_randomize_brk() is shared by x86 and x86_64, so if I moved it, it
would still carry the #ifdef CONFIG_X86_32.  I can certainly move it,
but I felt it was more readable closer to the brk-calculation logic of
the ELF loader.  (Nothing in the cs-limit patch[1] currently modifies
arch/x86/kernel/process.c, so I was also trying to avoid touching even
more files than it already does.)

Thanks,

-Kees

[1] http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-lucid.git;a=commitdiff;h=32a306aa2738c3d0a3f1c451b1a179358f02abf2

Patch

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b10acea..73594b9 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -978,6 +978,16 @@  static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 #ifdef arch_randomize_brk
 	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1))
 		current->mm->brk = current->mm->start_brk =
+# ifdef CONFIG_X86_32
+			/* in the case of NX emulation, shove the brk
+			   segment way out of the way of the exec
+			   randomization area, since it can collide with
+			   future allocations if not. */
+			( (current->mm->get_unmapped_exec_area ==
+			   arch_get_unmapped_exec_area) &&
+			  (current->mm->brk < 0x08000000)
+			  ? (TASK_SIZE/6) : 0) +
+# endif
 			arch_randomize_brk(current->mm);
 #endif
 
diff --git a/mm/mmap.c b/mm/mmap.c
index 814b95f..17ed65d 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1516,8 +1516,11 @@  arch_get_unmapped_exec_area(struct file *filp, unsigned long addr0,
 	if (flags & MAP_FIXED)
 		return addr;
 
-	if (!addr)
-		addr = randomize_range(SHLIB_BASE, 0x01000000, len);
+	if (!addr) {
+		addr = SHLIB_BASE;
+		if ((current->flags & PF_RANDOMIZE) && randomize_va_space)
+			addr = randomize_range(addr, 0x01000000, len);
+	}
 
 	if (addr) {
 		addr = PAGE_ALIGN(addr);
@@ -1545,7 +1548,9 @@  arch_get_unmapped_exec_area(struct file *filp, unsigned long addr0,
 			 * Up until the brk area we randomize addresses
 			 * as much as possible:
 			 */
-			if (addr >= 0x01000000) {
+			if ((current->flags & PF_RANDOMIZE) &&
+                            randomize_va_space &&
+                            addr >= 0x01000000) {
 				tmp = randomize_range(0x01000000,
 					PAGE_ALIGN(max(mm->start_brk,
 					(unsigned long)0x08000000)), len);