Errors in load_elf_binary()?

Errors in load_elf_binary()?

[Kirill Korotaev]

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

Re: Errors in load_elf_binary()?

[Chris Wright]

* Kirill Korotaev (kirillx@7ka.mipt.ru) 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