* Potential DOS in load_elf_library? @ 2005-03-18 8:15 Yichen Xie 2005-03-18 8:42 ` Andrew Morton 2005-03-18 8:52 ` Herbert Xu 0 siblings, 2 replies; 5+ messages in thread From: Yichen Xie @ 2005-03-18 8:15 UTC (permalink / raw) To: linux-kernel Hi guys, I was looking at the load_elf_library function (fs/binfmt_elf.c) in 2.6.10, and noticed the following: elf_phdata = (struct elf_phdr *) kmalloc(j, GFP_KERNEL); ... while (elf_phdata->p_type != PT_LOAD) elf_phdata++; ... kfree(elf_phdata); Could this be problematic since the pointer being freed might be different from that returned from kmalloc? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential DOS in load_elf_library? 2005-03-18 8:15 Potential DOS in load_elf_library? Yichen Xie @ 2005-03-18 8:42 ` Andrew Morton 2005-03-18 8:52 ` Herbert Xu 1 sibling, 0 replies; 5+ messages in thread From: Andrew Morton @ 2005-03-18 8:42 UTC (permalink / raw) To: Yichen Xie; +Cc: linux-kernel Yichen Xie <yxie@cs.stanford.edu> wrote: > > Hi guys, I was looking at the load_elf_library function (fs/binfmt_elf.c) > in 2.6.10, and noticed the following: > > elf_phdata = (struct elf_phdr *) kmalloc(j, GFP_KERNEL); > ... > while (elf_phdata->p_type != PT_LOAD) elf_phdata++; > ... > kfree(elf_phdata); > > Could this be problematic since the pointer being freed might be different > from that returned from kmalloc? Current kernels seem to be OK. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential DOS in load_elf_library? 2005-03-18 8:15 Potential DOS in load_elf_library? Yichen Xie 2005-03-18 8:42 ` Andrew Morton @ 2005-03-18 8:52 ` Herbert Xu 2005-03-18 9:05 ` Andrew Morton 1 sibling, 1 reply; 5+ messages in thread From: Herbert Xu @ 2005-03-18 8:52 UTC (permalink / raw) To: Yichen Xie; +Cc: linux-kernel, akpm Yichen Xie <yxie@cs.stanford.edu> wrote: > Hi guys, I was looking at the load_elf_library function (fs/binfmt_elf.c) > in 2.6.10, and noticed the following: > > elf_phdata = (struct elf_phdr *) kmalloc(j, GFP_KERNEL); > ... > while (elf_phdata->p_type != PT_LOAD) elf_phdata++; > ... > kfree(elf_phdata); > > Could this be problematic since the pointer being freed might be different > from that returned from kmalloc? Indeed. This bug has been around since last century. How does this look? Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- ===== fs/binfmt_elf.c 1.106 vs edited ===== --- 1.106/fs/binfmt_elf.c 2005-03-14 10:29:43 +11:00 +++ edited/fs/binfmt_elf.c 2005-03-18 19:46:37 +11:00 @@ -1026,6 +1026,7 @@ static int load_elf_library(struct file *file) { struct elf_phdr *elf_phdata; + struct elf_phdr *eppnt; unsigned long elf_bss, bss, len; int retval, error, i, j; struct elfhdr elf_ex; @@ -1063,30 +1064,32 @@ if (j != 1) goto out_free_ph; - while (elf_phdata->p_type != PT_LOAD) elf_phdata++; + eppnt = elf_phdata; + while (eppnt->p_type != PT_LOAD) + eppnt++; /* Now use mmap to map the library into memory. */ down_write(¤t->mm->mmap_sem); error = do_mmap(file, - ELF_PAGESTART(elf_phdata->p_vaddr), - (elf_phdata->p_filesz + - ELF_PAGEOFFSET(elf_phdata->p_vaddr)), + ELF_PAGESTART(eppnt->p_vaddr), + (eppnt->p_filesz + + ELF_PAGEOFFSET(eppnt->p_vaddr)), PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE, - (elf_phdata->p_offset - - ELF_PAGEOFFSET(elf_phdata->p_vaddr))); + (eppnt->p_offset - + ELF_PAGEOFFSET(eppnt->p_vaddr))); up_write(¤t->mm->mmap_sem); - if (error != ELF_PAGESTART(elf_phdata->p_vaddr)) + if (error != ELF_PAGESTART(eppnt->p_vaddr)) goto out_free_ph; - elf_bss = elf_phdata->p_vaddr + elf_phdata->p_filesz; + elf_bss = eppnt->p_vaddr + eppnt->p_filesz; if (padzero(elf_bss)) { error = -EFAULT; goto out_free_ph; } - len = ELF_PAGESTART(elf_phdata->p_filesz + elf_phdata->p_vaddr + ELF_MIN_ALIGN - 1); - bss = elf_phdata->p_memsz + elf_phdata->p_vaddr; + len = ELF_PAGESTART(eppnt->p_filesz + eppnt->p_vaddr + ELF_MIN_ALIGN - 1); + bss = eppnt->p_memsz + eppnt->p_vaddr; if (bss > len) { down_write(¤t->mm->mmap_sem); do_brk(len, bss - len); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential DOS in load_elf_library? 2005-03-18 8:52 ` Herbert Xu @ 2005-03-18 9:05 ` Andrew Morton 2005-03-18 9:09 ` Herbert Xu 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2005-03-18 9:05 UTC (permalink / raw) To: Herbert Xu; +Cc: yxie, linux-kernel Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Yichen Xie <yxie@cs.stanford.edu> wrote: > > Hi guys, I was looking at the load_elf_library function (fs/binfmt_elf.c) > > in 2.6.10, and noticed the following: > > > > elf_phdata = (struct elf_phdr *) kmalloc(j, GFP_KERNEL); > > ... > > while (elf_phdata->p_type != PT_LOAD) elf_phdata++; > > ... > > kfree(elf_phdata); > > > > Could this be problematic since the pointer being freed might be different > > from that returned from kmalloc? > > Indeed. This bug has been around since last century. Duh, I was looking at the wrong function. Thanks. > How does this look? I think it'd be better to use epptr everywhere, so we can see that it only gets assigned, tested then freed. --- 25/fs/binfmt_elf.c~load_elf_binary-kfree-fix 2005-03-18 01:00:49.000000000 -0800 +++ 25-akpm/fs/binfmt_elf.c 2005-03-18 01:03:14.000000000 -0800 @@ -1028,6 +1028,7 @@ out_free_ph: static int load_elf_library(struct file *file) { struct elf_phdr *elf_phdata; + struct elf_phdr *eppnt; unsigned long elf_bss, bss, len; int retval, error, i, j; struct elfhdr elf_ex; @@ -1051,44 +1052,47 @@ static int load_elf_library(struct file /* j < ELF_MIN_ALIGN because elf_ex.e_phnum <= 2 */ error = -ENOMEM; - elf_phdata = (struct elf_phdr *) kmalloc(j, GFP_KERNEL); + elf_phdata = kmalloc(j, GFP_KERNEL); if (!elf_phdata) goto out; + eppnt = elf_phdata; error = -ENOEXEC; - retval = kernel_read(file, elf_ex.e_phoff, (char *) elf_phdata, j); + retval = kernel_read(file, elf_ex.e_phoff, (char *)eppnt, j); if (retval != j) goto out_free_ph; for (j = 0, i = 0; i<elf_ex.e_phnum; i++) - if ((elf_phdata + i)->p_type == PT_LOAD) j++; + if ((eppnt + i)->p_type == PT_LOAD) + j++; if (j != 1) goto out_free_ph; - while (elf_phdata->p_type != PT_LOAD) elf_phdata++; + while (eppnt->p_type != PT_LOAD) + eppnt++; /* Now use mmap to map the library into memory. */ down_write(¤t->mm->mmap_sem); error = do_mmap(file, - ELF_PAGESTART(elf_phdata->p_vaddr), - (elf_phdata->p_filesz + - ELF_PAGEOFFSET(elf_phdata->p_vaddr)), + ELF_PAGESTART(eppnt->p_vaddr), + (eppnt->p_filesz + + ELF_PAGEOFFSET(eppnt->p_vaddr)), PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE, - (elf_phdata->p_offset - - ELF_PAGEOFFSET(elf_phdata->p_vaddr))); + (eppnt->p_offset - + ELF_PAGEOFFSET(eppnt->p_vaddr))); up_write(¤t->mm->mmap_sem); - if (error != ELF_PAGESTART(elf_phdata->p_vaddr)) + if (error != ELF_PAGESTART(eppnt->p_vaddr)) goto out_free_ph; - elf_bss = elf_phdata->p_vaddr + elf_phdata->p_filesz; + elf_bss = eppnt->p_vaddr + eppnt->p_filesz; if (padzero(elf_bss)) { error = -EFAULT; goto out_free_ph; } - len = ELF_PAGESTART(elf_phdata->p_filesz + elf_phdata->p_vaddr + ELF_MIN_ALIGN - 1); - bss = elf_phdata->p_memsz + elf_phdata->p_vaddr; + len = ELF_PAGESTART(eppnt->p_filesz + eppnt->p_vaddr + ELF_MIN_ALIGN - 1); + bss = eppnt->p_memsz + eppnt->p_vaddr; if (bss > len) { down_write(¤t->mm->mmap_sem); do_brk(len, bss - len); _ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential DOS in load_elf_library? 2005-03-18 9:05 ` Andrew Morton @ 2005-03-18 9:09 ` Herbert Xu 0 siblings, 0 replies; 5+ messages in thread From: Herbert Xu @ 2005-03-18 9:09 UTC (permalink / raw) To: Andrew Morton; +Cc: yxie, linux-kernel On Fri, Mar 18, 2005 at 01:05:01AM -0800, Andrew Morton wrote: > > I think it'd be better to use epptr everywhere, so we can see that it only > gets assigned, tested then freed. Looks good to me. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2005-03-18 9:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-03-18 8:15 Potential DOS in load_elf_library? Yichen Xie 2005-03-18 8:42 ` Andrew Morton 2005-03-18 8:52 ` Herbert Xu 2005-03-18 9:05 ` Andrew Morton 2005-03-18 9:09 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®