2004-04-08 21:26:21

by Kirill Korotaev

Subject: Errors in load_elf_binary()?

File: fs/binfmt_elf.c

1.

load_elf_binary()

2002/02/05 torvalds | retval = kernel_read(bprm->file, elf_ex.e_phoff, (char *) elf_phdata, size);
2002/02/05 torvalds | if (retval < 0)
2002/02/05 torvalds | goto out_free_ph;
2003/06/29 alan |
2003/06/29 alan | files = current->files; /* Refcounted so ok */
2003/06/29 alan | if(unshare_files() < 0)
2003/06/29 alan | goto out_free_ph;
<<<< retval is not set >>>>
should be something like:
retval = unshare_files()
if (retval < 0)
goto ....;
2003/08/09 agruen | if (files == current->files) {
2003/08/09 agruen | put_files_struct(files);
2003/08/09 agruen | files = NULL;
2003/08/09 agruen | }

........

2.

load_elf_binary()

2002/02/05 torvalds | out_free_dentry:
2002/02/05 torvalds | allow_write_access(interpreter);
2002/02/05 torvalds | fput(interpreter);
<<<< interpreter can be NULL >>>>
e.g. we got oopses here when flush_old_exec()
returns error
should be something like:
if (interpreter)
fput(interpreter);
2002/02/05 torvalds | out_free_interp:

3.

load_elf_binary()

Why there is no steal_locks() call in exit path (after label
"out_free_fh")? Shouldn't were steal locks back when undoing our changes?

Kirill

2004-04-08 22:11:22

by Chris Wright

Subject: Re: Errors in load_elf_binary()?

* Kirill Korotaev ([email protected]) wrote:
> File: fs/binfmt_elf.c
> 1.
> load_elf_binary()
> 2002/02/05 torvalds | retval = kernel_read(bprm->file, elf_ex.e_phoff, (char *) elf_phdata, size);
> 2002/02/05 torvalds | if (retval < 0)
> 2002/02/05 torvalds | goto out_free_ph;
> 2003/06/29 alan |
> 2003/06/29 alan | files = current->files; /* Refcounted so ok */
> 2003/06/29 alan | if(unshare_files() < 0)
> 2003/06/29 alan | goto out_free_ph;
> <<<< retval is not set >>>>
> should be something like:
> retval = unshare_files()
> if (retval < 0)
> goto ....;

yes, this looks like a bug.

> 2.
> load_elf_binary()
> 2002/02/05 torvalds | out_free_dentry:
> 2002/02/05 torvalds | allow_write_access(interpreter);
> 2002/02/05 torvalds | fput(interpreter);
> <<<< interpreter can be NULL >>>>
> e.g. we got oopses here when flush_old_exec()
> returns error
> should be something like:
> if (interpreter)
> fput(interpreter);

yup, this change is already in 2.6.

> 3.
> load_elf_binary()
> Why there is no steal_locks() call in exit path (after label
> "out_free_fh")? Shouldn't were steal locks back when undoing our changes?

No, on this error path locks never got stolen to begin with.

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