[PATCH 1/2] setup_arg_pages can insert overlapping vma

[PATCH 1/2] setup_arg_pages can insert overlapping vma

[Chris Wright]

Florian Heinz built an a.out binary that could map bss from 0x0 to
0xc0000000, and setup_arg_pages() would BUG() in insert_vma_struct
because the arg pages overlapped.  This just checks before inserting,
and bails out if it would overlap.

Signed-off-by: Chris Wright <chrisw@osdl.org>

===== fs/exec.c 1.143 vs edited =====
--- 1.143/fs/exec.c	2004-10-28 00:40:03 -07:00
+++ edited/fs/exec.c	2004-11-11 19:24:54 -08:00
@@ -413,6 +413,7 @@
 
 	down_write(&mm->mmap_sem);
 	{
+		struct vm_area_struct *vma;
 		mpnt->vm_mm = mm;
 #ifdef CONFIG_STACK_GROWSUP
 		mpnt->vm_start = stack_base;
@@ -433,6 +434,12 @@
 			mpnt->vm_flags = VM_STACK_FLAGS;
 		mpnt->vm_flags |= mm->def_flags;
 		mpnt->vm_page_prot = protection_map[mpnt->vm_flags & 0x7];
+		vma = find_vma(mm, mpnt->vm_start);
+		if (vma) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return -ENOMEM;
+		}
 		insert_vm_struct(mm, mpnt);
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	}

[PATCH 2/2] a.out: error check on set_brk

[Chris Wright]

It's possible for do_brk() to fail during set_brk() when exec'ing and
a.out.  This was noted with Florian's a.out binary and overcommit set
to 0.  Capture this error and terminate properly.

Signed-off-by: Chris Wright <chrisw@osdl.org>

===== fs/binfmt_aout.c 1.25 vs edited =====
--- 1.25/fs/binfmt_aout.c	2004-10-18 22:26:36 -07:00
+++ edited/fs/binfmt_aout.c	2004-11-11 22:28:58 -08:00
@@ -43,13 +43,18 @@
 	.min_coredump	= PAGE_SIZE
 };
 
-static void set_brk(unsigned long start, unsigned long end)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)
+
+static int set_brk(unsigned long start, unsigned long end)
 {
 	start = PAGE_ALIGN(start);
 	end = PAGE_ALIGN(end);
-	if (end <= start)
-		return;
-	do_brk(start, end - start);
+	if (end > start) {
+		unsigned long addr = do_brk(start, end - start);
+		if (BAD_ADDR(addr))
+			return addr;
+	}
+	return 0;
 }
 
 /*
@@ -413,7 +418,11 @@
 beyond_if:
 	set_binfmt(&aout_format);
 
-	set_brk(current->mm->start_brk, current->mm->brk);
+	retval = set_brk(current->mm->start_brk, current->mm->brk);
+	if (retval < 0) {
+		send_sig(SIGKILL, current, 0);
+		return retval;
+	}
 
 	retval = setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	if (retval < 0) { 

Re: [PATCH 1/2] setup_arg_pages can insert overlapping vma

[Hugh Dickins]

On Tue, 16 Nov 2004, Chris Wright wrote:
> Florian Heinz built an a.out binary that could map bss from 0x0 to
> 0xc0000000, and setup_arg_pages() would BUG() in insert_vma_struct
> because the arg pages overlapped.  This just checks before inserting,
> and bails out if it would overlap.

Chris, shouldn't your patch also cover the setup_arg_pages clones for
32-bit support on 64-bit architectures, with something - uncompiled,
untested - like the below?  I'm not sure how necessary the additional
vma->vm_start < mpnt->vm_end test is, but suspect ia64 might need it.

Signed-off-by: Hugh Dickins <hugh@veritas.com>

--- 2.6.10-rc2-bk2/arch/ia64/ia32/binfmt_elf32.c	2004-10-18 22:57:03.000000000 +0100
+++ linux/arch/ia64/ia32/binfmt_elf32.c	2004-11-18 17:17:57.000000000 +0000
@@ -214,6 +214,8 @@ ia32_setup_arg_pages (struct linux_binpr
 
 	down_write(&current->mm->mmap_sem);
 	{
+		struct vm_area_struct *vma;
+
 		mpnt->vm_mm = current->mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
@@ -225,6 +227,12 @@ ia32_setup_arg_pages (struct linux_binpr
 			mpnt->vm_flags = VM_STACK_FLAGS;
 		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC)?
 					PAGE_COPY_EXEC: PAGE_COPY;
+		vma = find_vma(current->mm, mpnt->vm_start);
+		if (vma && vma->vm_start < mpnt->vm_end) {
+			up_write(&current->mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return -ENOMEM;
+		}
 		insert_vm_struct(current->mm, mpnt);
 		current->mm->stack_vm = current->mm->total_vm = vma_pages(mpnt);
 	}
--- 2.6.10-rc2-bk2/arch/s390/kernel/compat_exec.c	2004-10-18 22:56:50.000000000 +0100
+++ linux/arch/s390/kernel/compat_exec.c	2004-11-18 17:17:57.000000000 +0000
@@ -62,12 +62,20 @@ int setup_arg_pages32(struct linux_binpr
 
 	down_write(&mm->mmap_sem);
 	{
+		struct vm_area_struct *vma;
+
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
 		/* executable stack setting would be applied here */
 		mpnt->vm_page_prot = PAGE_COPY;
 		mpnt->vm_flags = VM_STACK_FLAGS;
+		vma = find_vma(mm, mpnt->vm_start);
+		if (vma && vma->vm_start < mpnt->vm_end) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return -ENOMEM;
+		}
 		insert_vm_struct(mm, mpnt);
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	} 
--- 2.6.10-rc2-bk2/arch/x86_64/ia32/ia32_binfmt.c	2004-11-15 16:20:34.000000000 +0000
+++ linux/arch/x86_64/ia32/ia32_binfmt.c	2004-11-18 17:17:57.000000000 +0000
@@ -357,6 +357,8 @@ int setup_arg_pages(struct linux_binprm 
 
 	down_write(&mm->mmap_sem);
 	{
+		struct vm_area_struct *vma;
+
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
@@ -368,6 +370,12 @@ int setup_arg_pages(struct linux_binprm 
 			mpnt->vm_flags = VM_STACK_FLAGS;
  		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC) ? 
  			PAGE_COPY_EXEC : PAGE_COPY;
+		vma = find_vma(mm, mpnt->vm_start);
+		if (vma && vma->vm_start < mpnt->vm_end) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return -ENOMEM;
+		}
 		insert_vm_struct(mm, mpnt);
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	} 

Re: [PATCH 1/2] setup_arg_pages can insert overlapping vma

[Chris Wright]

* Hugh Dickins (hugh@veritas.com) wrote:
> On Tue, 16 Nov 2004, Chris Wright wrote:
> > Florian Heinz built an a.out binary that could map bss from 0x0 to
> > 0xc0000000, and setup_arg_pages() would BUG() in insert_vma_struct
> > because the arg pages overlapped.  This just checks before inserting,
> > and bails out if it would overlap.
> 
> Chris, shouldn't your patch also cover the setup_arg_pages clones for
> 32-bit support on 64-bit architectures, with something - uncompiled,
> untested - like the below?  I'm not sure how necessary the additional
> vma->vm_start < mpnt->vm_end test is, but suspect ia64 might need it.

I expect other arches should need the fix as well, it would be nice
to test them.  I'm not clear on that extra test.  Wouldn't it imply
vm_end < vm_start?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH 1/2] setup_arg_pages can insert overlapping vma

[Hugh Dickins]

On Thu, 18 Nov 2004, Chris Wright wrote:
> * Hugh Dickins (hugh@veritas.com) wrote:
> > 
> > Chris, shouldn't your patch also cover the setup_arg_pages clones for
> > 32-bit support on 64-bit architectures, with something - uncompiled,
> > untested - like the below?  I'm not sure how necessary the additional
> > vma->vm_start < mpnt->vm_end test is, but suspect ia64 might need it.
> 
> I expect other arches should need the fix as well, it would be nice
> to test them.

ia64, s390 and x86_64 seem to be the only ones with their own code
to insert_vm_struct for 32-bit setup_arg_pages.

> I'm not clear on that extra test.  Wouldn't it imply vm_end < vm_start?

Whose vm_end and whose vm_start?  Well, no need to answer...

Check the comment on find_vma in mm/mmap.c:
/* Look up the first VMA which satisfies  addr < vm_end,  NULL if none. */
but perhaps you thought it returns NULL if addr is not covered by a vma?

If so, maybe your original fs/exec.c fix also needs that check added:
it's usually the case that there cannot be a vma above the stack being
set up here, but I don't know enough of all the architectures to say
that's always so (and it looks like not the case for 32-bit on ia64).

Hugh

Re: [PATCH 1/2] setup_arg_pages can insert overlapping vma

[Chris Wright]

* Hugh Dickins (hugh@veritas.com) wrote:
> Check the comment on find_vma in mm/mmap.c:
> /* Look up the first VMA which satisfies  addr < vm_end,  NULL if none. */
> but perhaps you thought it returns NULL if addr is not covered by a vma?

Ah, yes, being at top of stack was part of my assumption.  But I see
your point.  I think find_vma_intersection() might make best sense then.

thanks,
-chris