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

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

[Zou, Nanhai]

[-- Attachment #1: Type: text/plain, Size: 5060 bytes --]

 <<ia64-vm-overlap.tar.gz>>  <<vma-overlap-fix.patch>> I think ia64 ia32
subsystem is not vulnerable to this kind of overlapping vm problem,
because it does not support a.out binary format, 
X84_64 is vulnerable to this. 

just do a 
perl -e'print"\x07\x01".("\x00"x10)."\x00\xe0\xff\xff".("\x00"x16)'>
evilaout
you will get it.
 
and IA64 is also vulnerable to this kind of bug in 64 bit elf support,
it just insert a vma of zero page without checking overlap, so user can
construct a elf with section begin from 0x0 to trigger this BUGON().I
attach a testcase to trigger this bug
I don't know what about s390. However, I think it's safe to check
overlap before we actually insert a vma into vma list.
 
And I also feel check vma overlap everywhere is unnecessary, because
invert_vm_struct will check it again, so the check is duplicated. It's
better to have invert_vm_struct return a value then let caller check if
it successes.
Here is a patch against 2.6.10.rc2-mm3
I have tested it on i386, x86_64 and ia64 machines.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Zou Nan hai <Nanhai.zou@intel.com>


-----Original Message-----
From: linux-kernel-owner@vger.kernel.org
[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Hugh Dickins
Sent: Friday, November 19, 2004 1:40 AM
To: Chris Wright
Cc: Andrew Morton; Linus Torvalds; Luck, Tony; Martin Schwidefsky; Andi
Kleen; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] setup_arg_pages can insert overlapping vma

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[-- Attachment #2: ia64-vm-overlap.tar.gz --]
[-- Type: application/x-gzip, Size: 2741 bytes --]

[-- Attachment #3: vma-overlap-fix.patch --]
[-- Type: application/octet-stream, Size: 7815 bytes --]

diff -Nraup a/arch/ia64/ia32/binfmt_elf32.c b/arch/ia64/ia32/binfmt_elf32.c
--- a/arch/ia64/ia32/binfmt_elf32.c	2004-11-21 23:58:21.344700219 -0800
+++ b/arch/ia64/ia32/binfmt_elf32.c	2004-11-21 23:58:45.143528053 -0800
@@ -100,7 +100,11 @@ ia64_elf32_init (struct pt_regs *regs)
 		vma->vm_ops = &ia32_shared_page_vm_ops;
 		down_write(&current->mm->mmap_sem);
 		{
-			insert_vm_struct(current->mm, vma);
+			if (insert_vm_struct(current->mm, vma)) {
+				kmem_cache_free(vm_area_cachep, vma);
+				up_write(&current->mm->mmap_sem);
+				return;
+			}
 		}
 		up_write(&current->mm->mmap_sem);
 	}
@@ -123,7 +127,11 @@ ia64_elf32_init (struct pt_regs *regs)
 		vma->vm_ops = &ia32_gate_page_vm_ops;
 		down_write(&current->mm->mmap_sem);
 		{
-			insert_vm_struct(current->mm, vma);
+			if (insert_vm_struct(current->mm, vma)) {
+				kmem_cache_free(vm_area_cachep, vma);
+				up_write(&current->mm->mmap_sem);
+				return;
+			}
 		}
 		up_write(&current->mm->mmap_sem);
 	}
@@ -142,7 +150,11 @@ ia64_elf32_init (struct pt_regs *regs)
 		vma->vm_flags = VM_READ|VM_WRITE|VM_MAYREAD|VM_MAYWRITE;
 		down_write(&current->mm->mmap_sem);
 		{
-			insert_vm_struct(current->mm, vma);
+			if (insert_vm_struct(current->mm, vma)) {
+				kmem_cache_free(vm_area_cachep, vma);
+				up_write(&current->mm->mmap_sem);
+				return;
+			}
 		}
 		up_write(&current->mm->mmap_sem);
 	}
@@ -190,7 +202,7 @@ ia32_setup_arg_pages (struct linux_binpr
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 
 	stack_base = IA32_STACK_TOP - MAX_ARG_PAGES*PAGE_SIZE;
 	mm->arg_start = bprm->p + stack_base;
@@ -225,7 +237,11 @@ 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;
-		insert_vm_struct(current->mm, mpnt);
+		if ((ret = insert_vm_struct(current->mm, mpnt))) {
+			up_write(&current->mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return ret;
+		}
 		current->mm->stack_vm = current->mm->total_vm = vma_pages(mpnt);
 	}
 
diff -Nraup a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
--- a/arch/ia64/mm/init.c	2004-11-21 23:58:21.345676782 -0800
+++ b/arch/ia64/mm/init.c	2004-11-22 00:00:54.527315530 -0800
@@ -131,7 +131,13 @@ ia64_init_addr_space (void)
 		vma->vm_end = vma->vm_start + PAGE_SIZE;
 		vma->vm_page_prot = protection_map[VM_DATA_DEFAULT_FLAGS & 0x7];
 		vma->vm_flags = VM_DATA_DEFAULT_FLAGS | VM_GROWSUP;
-		insert_vm_struct(current->mm, vma);
+		down_write(&current->mm->mmap_sem);
+		if (insert_vm_struct(current->mm, vma)) {
+			up_write(&current->mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, vma);
+			return;
+		}
+		up_write(&current->mm->mmap_sem);
 	}
 
 	/* map NaT-page at address zero to speed up speculative dereferencing of NULL: */
@@ -143,7 +149,13 @@ ia64_init_addr_space (void)
 			vma->vm_end = PAGE_SIZE;
 			vma->vm_page_prot = __pgprot(pgprot_val(PAGE_READONLY) | _PAGE_MA_NAT);
 			vma->vm_flags = VM_READ | VM_MAYREAD | VM_IO | VM_RESERVED;
-			insert_vm_struct(current->mm, vma);
+			down_write(&current->mm->mmap_sem);
+			if (insert_vm_struct(current->mm, vma)) {
+				up_write(&current->mm->mmap_sem);
+				kmem_cache_free(vm_area_cachep, vma);
+				return;
+			}
+			up_write(&current->mm->mmap_sem);
 		}
 	}
 }
diff -Nraup a/arch/s390/kernel/compat_exec.c b/arch/s390/kernel/compat_exec.c
--- a/arch/s390/kernel/compat_exec.c	2004-11-21 23:58:21.428684593 -0800
+++ b/arch/s390/kernel/compat_exec.c	2004-11-21 23:58:45.144504615 -0800
@@ -39,7 +39,7 @@ int setup_arg_pages32(struct linux_binpr
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 
 	stack_base = STACK_TOP - MAX_ARG_PAGES*PAGE_SIZE;
 	mm->arg_start = bprm->p + stack_base;
@@ -68,7 +68,11 @@ int setup_arg_pages32(struct linux_binpr
 		/* executable stack setting would be applied here */
 		mpnt->vm_page_prot = PAGE_COPY;
 		mpnt->vm_flags = VM_STACK_FLAGS;
-		insert_vm_struct(mm, mpnt);
+		if ((ret = insert_vm_struct(mm, mpnt))) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return ret;
+		}
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	} 
 
diff -Nraup a/arch/x86_64/ia32/ia32_binfmt.c b/arch/x86_64/ia32/ia32_binfmt.c
--- a/arch/x86_64/ia32/ia32_binfmt.c	2004-11-21 23:58:21.423801781 -0800
+++ b/arch/x86_64/ia32/ia32_binfmt.c	2004-11-21 23:58:45.145481178 -0800
@@ -335,7 +335,7 @@ int setup_arg_pages(struct linux_binprm 
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 
 	stack_base = IA32_STACK_TOP - MAX_ARG_PAGES * PAGE_SIZE;
 	mm->arg_start = bprm->p + stack_base;
@@ -369,7 +369,11 @@ 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;
-		insert_vm_struct(mm, mpnt);
+		if ((ret = insert_vm_struct(mm, mpnt))) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return ret;
+		}
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	} 
 
diff -Nraup a/fs/exec.c b/fs/exec.c
--- a/fs/exec.c	2004-11-21 23:58:27.971653263 -0800
+++ b/fs/exec.c	2004-11-22 00:25:39.885695772 -0800
@@ -351,7 +351,7 @@ int setup_arg_pages(struct linux_binprm 
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 	long arg_size;
 
 #ifdef CONFIG_STACK_GROWSUP
@@ -424,7 +424,6 @@ int setup_arg_pages(struct linux_binprm 
 
 	down_write(&mm->mmap_sem);
 	{
-		struct vm_area_struct *vma;
 		mpnt->vm_mm = mm;
 #ifdef CONFIG_STACK_GROWSUP
 		mpnt->vm_start = stack_base;
@@ -444,13 +443,11 @@ int setup_arg_pages(struct linux_binprm 
 			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) {
+		if ((ret = insert_vm_struct(mm, mpnt))) {
 			up_write(&mm->mmap_sem);
 			kmem_cache_free(vm_area_cachep, mpnt);
-			return -ENOMEM;
+			return ret;
 		}
-		insert_vm_struct(mm, mpnt);
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	}
 
diff -Nraup a/include/linux/mm.h b/include/linux/mm.h
--- a/include/linux/mm.h	2004-11-21 23:58:21.575168966 -0800
+++ b/include/linux/mm.h	2004-11-21 23:58:45.147434303 -0800
@@ -743,7 +743,7 @@ extern struct vm_area_struct *vma_merge(
 extern struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *);
 extern int split_vma(struct mm_struct *,
 	struct vm_area_struct *, unsigned long addr, int new_below);
-extern void insert_vm_struct(struct mm_struct *, struct vm_area_struct *);
+extern int insert_vm_struct(struct mm_struct *, struct vm_area_struct *);
 extern void __vma_link_rb(struct mm_struct *, struct vm_area_struct *,
 	struct rb_node **, struct rb_node *);
 extern struct vm_area_struct *copy_vma(struct vm_area_struct **,
diff -Nraup a/mm/mmap.c b/mm/mmap.c
--- a/mm/mmap.c	2004-11-21 23:58:29.059543875 -0800
+++ b/mm/mmap.c	2004-11-21 23:58:45.149387428 -0800
@@ -1913,7 +1913,7 @@ void exit_mmap(struct mm_struct *mm)
  * and into the inode's i_mmap tree.  If vm_file is non-NULL
  * then i_mmap_lock is taken here.
  */
-void insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
+int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
 {
 	struct vm_area_struct * __vma, * prev;
 	struct rb_node ** rb_link, * rb_parent;
@@ -1936,8 +1936,9 @@ void insert_vm_struct(struct mm_struct *
 	}
 	__vma = find_vma_prepare(mm,vma->vm_start,&prev,&rb_link,&rb_parent);
 	if (__vma && __vma->vm_start < vma->vm_end)
-		BUG();
+		return -ENOMEM;
 	vma_link(mm, vma, prev, rb_link, rb_parent);
+	return 0;
 }
 
 /*

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

[Andrew Morton]

"Zou, Nanhai" <nanhai.zou@intel.com> wrote:
>
> I think ia64 ia32
> subsystem is not vulnerable to this kind of overlapping vm problem,
> because it does not support a.out binary format, 
> X84_64 is vulnerable to this. 

Martin, Andi and Tony: could we please get a 2.6.10 ack on this from you?

Thanks.

(It sounds like s390 needs more work?)

From: "Zou, Nanhai" <nanhai.zou@intel.com>

IA64 is also vulnerable to the huge-vma-in-executable bug in 64 bit elf
support, it just insert a vma of zero page without checking overlap, so user
can construct a elf with section begin from 0x0 to trigger this BUGON().

I attach a testcase to trigger this bug I don't know what about s390.

However, I think it's safe to check overlap before we actually insert a vma
into vma list.  And I also feel check vma overlap everywhere is unnecessary,
because invert_vm_struct will check it again, so the check is duplicated. 
It's better to have invert_vm_struct return a value then let caller check if
it successes.  Here is a patch against 2.6.10.rc2-mm3 I have tested it on
i386, x86_64 and ia64 machines.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Zou Nan hai <Nanhai.zou@intel.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/arch/ia64/ia32/binfmt_elf32.c  |   26 +++++++++++++++++++++-----
 25-akpm/arch/ia64/mm/init.c            |   16 ++++++++++++++--
 25-akpm/arch/s390/kernel/compat_exec.c |    8 ++++++--
 25-akpm/arch/x86_64/ia32/ia32_binfmt.c |    8 ++++++--
 25-akpm/fs/exec.c                      |    9 +++------
 25-akpm/include/linux/mm.h             |    2 +-
 25-akpm/mm/mmap.c                      |    5 +++--
 7 files changed, 54 insertions(+), 20 deletions(-)

diff -puN arch/ia64/ia32/binfmt_elf32.c~ia64-overlapping-vma-fix arch/ia64/ia32/binfmt_elf32.c
--- 25/arch/ia64/ia32/binfmt_elf32.c~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/arch/ia64/ia32/binfmt_elf32.c	Tue Nov 23 17:21:30 2004
@@ -100,7 +100,11 @@ ia64_elf32_init (struct pt_regs *regs)
 		vma->vm_ops = &ia32_shared_page_vm_ops;
 		down_write(&current->mm->mmap_sem);
 		{
-			insert_vm_struct(current->mm, vma);
+			if (insert_vm_struct(current->mm, vma)) {
+				kmem_cache_free(vm_area_cachep, vma);
+				up_write(&current->mm->mmap_sem);
+				return;
+			}
 		}
 		up_write(&current->mm->mmap_sem);
 	}
@@ -123,7 +127,11 @@ ia64_elf32_init (struct pt_regs *regs)
 		vma->vm_ops = &ia32_gate_page_vm_ops;
 		down_write(&current->mm->mmap_sem);
 		{
-			insert_vm_struct(current->mm, vma);
+			if (insert_vm_struct(current->mm, vma)) {
+				kmem_cache_free(vm_area_cachep, vma);
+				up_write(&current->mm->mmap_sem);
+				return;
+			}
 		}
 		up_write(&current->mm->mmap_sem);
 	}
@@ -142,7 +150,11 @@ ia64_elf32_init (struct pt_regs *regs)
 		vma->vm_flags = VM_READ|VM_WRITE|VM_MAYREAD|VM_MAYWRITE;
 		down_write(&current->mm->mmap_sem);
 		{
-			insert_vm_struct(current->mm, vma);
+			if (insert_vm_struct(current->mm, vma)) {
+				kmem_cache_free(vm_area_cachep, vma);
+				up_write(&current->mm->mmap_sem);
+				return;
+			}
 		}
 		up_write(&current->mm->mmap_sem);
 	}
@@ -190,7 +202,7 @@ ia32_setup_arg_pages (struct linux_binpr
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 
 	stack_base = IA32_STACK_TOP - MAX_ARG_PAGES*PAGE_SIZE;
 	mm->arg_start = bprm->p + stack_base;
@@ -225,7 +237,11 @@ 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;
-		insert_vm_struct(current->mm, mpnt);
+		if ((ret = insert_vm_struct(current->mm, mpnt))) {
+			up_write(&current->mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return ret;
+		}
 		current->mm->stack_vm = current->mm->total_vm = vma_pages(mpnt);
 	}
 
diff -puN arch/ia64/mm/init.c~ia64-overlapping-vma-fix arch/ia64/mm/init.c
--- 25/arch/ia64/mm/init.c~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/arch/ia64/mm/init.c	Tue Nov 23 17:21:30 2004
@@ -131,7 +131,13 @@ ia64_init_addr_space (void)
 		vma->vm_end = vma->vm_start + PAGE_SIZE;
 		vma->vm_page_prot = protection_map[VM_DATA_DEFAULT_FLAGS & 0x7];
 		vma->vm_flags = VM_DATA_DEFAULT_FLAGS | VM_GROWSUP;
-		insert_vm_struct(current->mm, vma);
+		down_write(&current->mm->mmap_sem);
+		if (insert_vm_struct(current->mm, vma)) {
+			up_write(&current->mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, vma);
+			return;
+		}
+		up_write(&current->mm->mmap_sem);
 	}
 
 	/* map NaT-page at address zero to speed up speculative dereferencing of NULL: */
@@ -143,7 +149,13 @@ ia64_init_addr_space (void)
 			vma->vm_end = PAGE_SIZE;
 			vma->vm_page_prot = __pgprot(pgprot_val(PAGE_READONLY) | _PAGE_MA_NAT);
 			vma->vm_flags = VM_READ | VM_MAYREAD | VM_IO | VM_RESERVED;
-			insert_vm_struct(current->mm, vma);
+			down_write(&current->mm->mmap_sem);
+			if (insert_vm_struct(current->mm, vma)) {
+				up_write(&current->mm->mmap_sem);
+				kmem_cache_free(vm_area_cachep, vma);
+				return;
+			}
+			up_write(&current->mm->mmap_sem);
 		}
 	}
 }
diff -puN arch/s390/kernel/compat_exec.c~ia64-overlapping-vma-fix arch/s390/kernel/compat_exec.c
--- 25/arch/s390/kernel/compat_exec.c~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/arch/s390/kernel/compat_exec.c	Tue Nov 23 17:21:30 2004
@@ -39,7 +39,7 @@ int setup_arg_pages32(struct linux_binpr
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 
 	stack_base = STACK_TOP - MAX_ARG_PAGES*PAGE_SIZE;
 	mm->arg_start = bprm->p + stack_base;
@@ -68,7 +68,11 @@ int setup_arg_pages32(struct linux_binpr
 		/* executable stack setting would be applied here */
 		mpnt->vm_page_prot = PAGE_COPY;
 		mpnt->vm_flags = VM_STACK_FLAGS;
-		insert_vm_struct(mm, mpnt);
+		if ((ret = insert_vm_struct(mm, mpnt))) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return ret;
+		}
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	} 
 
diff -puN arch/x86_64/ia32/ia32_binfmt.c~ia64-overlapping-vma-fix arch/x86_64/ia32/ia32_binfmt.c
--- 25/arch/x86_64/ia32/ia32_binfmt.c~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/arch/x86_64/ia32/ia32_binfmt.c	Tue Nov 23 17:21:30 2004
@@ -334,7 +334,7 @@ int setup_arg_pages(struct linux_binprm 
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 
 	stack_base = IA32_STACK_TOP - MAX_ARG_PAGES * PAGE_SIZE;
 	mm->arg_start = bprm->p + stack_base;
@@ -368,7 +368,11 @@ 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;
-		insert_vm_struct(mm, mpnt);
+		if ((ret = insert_vm_struct(mm, mpnt))) {
+			up_write(&mm->mmap_sem);
+			kmem_cache_free(vm_area_cachep, mpnt);
+			return ret;
+		}
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	} 
 
diff -puN fs/exec.c~ia64-overlapping-vma-fix fs/exec.c
--- 25/fs/exec.c~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/fs/exec.c	Tue Nov 23 17:21:30 2004
@@ -342,7 +342,7 @@ int setup_arg_pages(struct linux_binprm 
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
 	struct mm_struct *mm = current->mm;
-	int i;
+	int i, ret;
 	long arg_size;
 
 #ifdef CONFIG_STACK_GROWSUP
@@ -413,7 +413,6 @@ int setup_arg_pages(struct linux_binprm 
 
 	down_write(&mm->mmap_sem);
 	{
-		struct vm_area_struct *vma;
 		mpnt->vm_mm = mm;
 #ifdef CONFIG_STACK_GROWSUP
 		mpnt->vm_start = stack_base;
@@ -434,13 +433,11 @@ int setup_arg_pages(struct linux_binprm 
 			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) {
+		if ((ret = insert_vm_struct(mm, mpnt))) {
 			up_write(&mm->mmap_sem);
 			kmem_cache_free(vm_area_cachep, mpnt);
-			return -ENOMEM;
+			return ret;
 		}
-		insert_vm_struct(mm, mpnt);
 		mm->stack_vm = mm->total_vm = vma_pages(mpnt);
 	}
 
diff -puN include/linux/mm.h~ia64-overlapping-vma-fix include/linux/mm.h
--- 25/include/linux/mm.h~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/include/linux/mm.h	Tue Nov 23 17:21:30 2004
@@ -675,7 +675,7 @@ extern struct vm_area_struct *vma_merge(
 extern struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *);
 extern int split_vma(struct mm_struct *,
 	struct vm_area_struct *, unsigned long addr, int new_below);
-extern void insert_vm_struct(struct mm_struct *, struct vm_area_struct *);
+extern int insert_vm_struct(struct mm_struct *, struct vm_area_struct *);
 extern void __vma_link_rb(struct mm_struct *, struct vm_area_struct *,
 	struct rb_node **, struct rb_node *);
 extern struct vm_area_struct *copy_vma(struct vm_area_struct **,
diff -puN mm/mmap.c~ia64-overlapping-vma-fix mm/mmap.c
--- 25/mm/mmap.c~ia64-overlapping-vma-fix	Tue Nov 23 17:21:30 2004
+++ 25-akpm/mm/mmap.c	Tue Nov 23 17:21:30 2004
@@ -1871,7 +1871,7 @@ void exit_mmap(struct mm_struct *mm)
  * and into the inode's i_mmap tree.  If vm_file is non-NULL
  * then i_mmap_lock is taken here.
  */
-void insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
+int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
 {
 	struct vm_area_struct * __vma, * prev;
 	struct rb_node ** rb_link, * rb_parent;
@@ -1894,8 +1894,9 @@ void insert_vm_struct(struct mm_struct *
 	}
 	__vma = find_vma_prepare(mm,vma->vm_start,&prev,&rb_link,&rb_parent);
 	if (__vma && __vma->vm_start < vma->vm_end)
-		BUG();
+		return -ENOMEM;
 	vma_link(mm, vma, prev, rb_link, rb_parent);
+	return 0;
 }
 
 /*
_

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

[Andi Kleen]

On Tue, Nov 23, 2004 at 05:23:09PM -0800, Andrew Morton wrote:
> "Zou, Nanhai" <nanhai.zou@intel.com> wrote:
> >
> > I think ia64 ia32
> > subsystem is not vulnerable to this kind of overlapping vm problem,
> > because it does not support a.out binary format, 
> > X84_64 is vulnerable to this. 
> 
> Martin, Andi and Tony: could we please get a 2.6.10 ack on this from you?

Looks good to me.

-Andi

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

[Hugh Dickins]

Thanks a lot for taking this further.

On Wed, 24 Nov 2004, Zou, Nanhai wrote:
> I think ia64 ia32
> subsystem is not vulnerable to this kind of overlapping vm problem,
> because it does not support a.out binary format, 
> X84_64 is vulnerable to this. 
> 
> just do a 
> perl -e'print"\x07\x01".("\x00"x10)."\x00\xe0\xff\xff".("\x00"x16)'>
> evilaout
> you will get it.
>  
> and IA64 is also vulnerable to this kind of bug in 64 bit elf support,
> it just insert a vma of zero page without checking overlap, so user can
> construct a elf with section begin from 0x0 to trigger this BUGON().I
> attach a testcase to trigger this bug
> I don't know what about s390. However, I think it's safe to check
> overlap before we actually insert a vma into vma list.

I expect you're right: I have neither machines nor expertise to say.

> And I also feel check vma overlap everywhere is unnecessary, because
> invert_vm_struct will check it again, so the check is duplicated. It's
> better to have invert_vm_struct return a value then let caller check if
> it successes.
> Here is a patch against 2.6.10.rc2-mm3
> I have tested it on i386, x86_64 and ia64 machines.

Yes, I agree, that's a welcome improvement.  I'm surprised if all
those ia64_elf32_init checks are necessary, but better safe than sorry.

Something crosses my mind, you'll know better than I: is it possible to
construct ELFs or A.OUTs which would need the check in insert_vm_struct
to be even more defensive?  That is, should it also be checking that
vma->vm_end > vma->vm_start (vma being the one to be inserted)?
Or that vma->vm_end <= TASK_SIZE?  If I remember rightly, a 0-length
vma can cause confusion but survive quite well until exit_mmap's
BUG_ON(mm->map_count).

Hugh

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

[Chris Wright]

* Zou, Nanhai (nanhai.zou@intel.com) wrote:
>  <<ia64-vm-overlap.tar.gz>>  <<vma-overlap-fix.patch>> I think ia64 ia32
> subsystem is not vulnerable to this kind of overlapping vm problem,
> because it does not support a.out binary format, 

I am able to map a section over the arg pages, and for some reason this
case segfaults (in the application).  Disassembly shows garbage left
behind in that page.  AFAICT, this can only cause the app to segfault in
userspace.

> X84_64 is vulnerable to this. 
> 
> just do a 
> perl -e'print"\x07\x01".("\x00"x10)."\x00\xe0\xff\xff".("\x00"x16)'>
> evilaout
> you will get it.
>  
> and IA64 is also vulnerable to this kind of bug in 64 bit elf support,
> it just insert a vma of zero page without checking overlap, so user can
> construct a elf with section begin from 0x0 to trigger this BUGON().I
> attach a testcase to trigger this bug

Yes, I was able to reproduce a similar bug last night on ia64 by placing
a 1k section at 0x1000, and this patch indeed fixes it up.

> I don't know what about s390. However, I think it's safe to check
> overlap before we actually insert a vma into vma list.
>  
> And I also feel check vma overlap everywhere is unnecessary, because
> invert_vm_struct will check it again, so the check is duplicated. It's
> better to have invert_vm_struct return a value then let caller check if
> it successes.

Yes I agree.  That's the question I asked early on.  With no answer I
took defensive route to be sure the BUG() wasn't there for some valid
reason I was missing.  This looks better.

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

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

[Zou, Nanhai]

> -----Original Message-----
> From: Hugh Dickins [mailto:hugh@veritas.com]
> Sent: Thursday, November 25, 2004 12:31 AM
> To: Zou, Nanhai
> Cc: Chris Wright; Andrew Morton; Linus Torvalds; Luck, Tony; Martin
Schwidefsky;
> Andi Kleen; linux-kernel@vger.kernel.org; linux-ia64@vger.kernel.org
> Subject: RE: [PATCH 1/2] setup_arg_pages can insert overlapping vma
> 
> Thanks a lot for taking this further.
> 
> Yes, I agree, that's a welcome improvement.  I'm surprised if all
> those ia64_elf32_init checks are necessary, but better safe than
sorry.
> 
> Something crosses my mind, you'll know better than I: is it possible
to
> construct ELFs or A.OUTs which would need the check in
insert_vm_struct
> to be even more defensive?  That is, should it also be checking that
> vma->vm_end > vma->vm_start (vma being the one to be inserted)?
> Or that vma->vm_end <= TASK_SIZE?  If I remember rightly, a 0-length
> vma can cause confusion but survive quite well until exit_mmap's
> BUG_ON(mm->map_count).
  Since all elf and a.out sections are inserted with do_mmap which takes
start_addr and an unsigned length as parameters. And do_mmap also check
for zero lenth mapping.
I think we could not have vma with (vma->vm_end <= vm->vm_start) by
construct a bad binary executable.

Zou Nan hai 

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

[Luck, Tony]

>"Zou, Nanhai" <nanhai.zou@intel.com> wrote:
>>
>> I think ia64 ia32
>> subsystem is not vulnerable to this kind of overlapping vm problem,
>> because it does not support a.out binary format, 
>> X84_64 is vulnerable to this. 
>
>Martin, Andi and Tony: could we please get a 2.6.10 ack on 
>this from you?

I can "Ack" it too ... but I gave Nanhai one of my "Signed-off-by:"
lines which he already attached to this patch :-)

-Tony

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

[Hugh Dickins]

On Wed, 24 Nov 2004, Martin Schwidefsky wrote:
> 
> In principle the patch is fine (it works and fixes a problem).
> 
> I tried to find out why s390 needs its own setup_arg_pages32 function.
> After I've compared the function with the common setup_arg_pages several
> times and still couldn't find a reason for it I just removed the function.
> Still works fine. I think the reason to introduce setup_arg_pages32 has
> been the STACK_TOP define which needs to reflect the smaller address
> space for 31 bit programs. Since the change that made TASK_SIZE look
> at the TIF_31BIT bit to return the correct value for 31 and 64 bit
> programs STACK_TOP is just correct as well. 
> 
> In short, just remove setup_arg_pages32.

Tell me I'm being silly, Martin, but wouldn't it be wiser to stick with
setup_arg_pages32 (with Nanhai's patch) for 2.6.10, then remove it after?

I'm cautious here because about six months ago I sent Andrew a patch
which eliminated setup_arg_pages32 (or equiv) from the three arches,
but the x86_64 one just wouldn't boot on Andrew's machine.  Both hch
and I spent hours staring at the code, neither of us could work out why.

Now, s390 may be greatly superior ;) but all the setup_arg_pages32
redefining is twisty weird stuff - good to be rid of it, but right now?

Hugh

[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);
 	}

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