2005-03-18 08:15:56

by Yichen Xie

Subject: Potential DOS in load_elf_library?

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?

2005-03-18 08:42:41

by Andrew Morton

Subject: Re: Potential DOS in load_elf_library?

Yichen Xie <[email protected]> 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.

2005-03-18 08:54:23

by Herbert Xu

Subject: Re: Potential DOS in load_elf_library?

Yichen Xie <[email protected]> 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~} <[email protected]>
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);

2005-03-18 09:08:14

by Andrew Morton

Subject: Re: Potential DOS in load_elf_library?

Herbert Xu <[email protected]> wrote:
>
> Yichen Xie <[email protected]> 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);
_

2005-03-18 09:11:16

by Herbert Xu

Subject: Re: Potential DOS in load_elf_library?

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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt