Potential DOS in load_elf_library?

Potential DOS in load_elf_library?

[Yichen Xie]

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?

Re: Potential DOS in load_elf_library?

[Andrew Morton]

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.

Re: Potential DOS in load_elf_library?

[Herbert Xu]

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(&current->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(&current->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(&current->mm->mmap_sem);
 		do_brk(len, bss - len);

Re: Potential DOS in load_elf_library?

[Andrew Morton]

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(&current->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(&current->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(&current->mm->mmap_sem);
 		do_brk(len, bss - len);
_

Re: Potential DOS in load_elf_library?

[Herbert Xu]

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