Messages in this thread | | | Date | Sat, 3 Aug 2013 12:27:22 -0700 | Subject | Re: [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm() | From | Kees Cook <> |
| |
On Fri, Aug 2, 2013 at 12:27 PM, Oleg Nesterov <oleg@redhat.com> wrote: > When search_binary_handler() succeeds it does allow_write_access() > and fput(), then it clears bprm->file to ensure the caller will not > do the same. > > We can simply move this code to exec_binprm() which is called only > once. In fact we could move this to free_bprm() and remove the same > code in do_execve_common's error path. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > fs/exec.c | 9 +++++---- > 1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index ad7d624..ef70320 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1400,10 +1400,6 @@ int search_binary_handler(struct linux_binprm *bprm) > bprm->recursion_depth--; > if (retval >= 0) { > put_binfmt(fmt); > - allow_write_access(bprm->file); > - if (bprm->file) > - fput(bprm->file); > - bprm->file = NULL; > return retval; > } > read_lock(&binfmt_lock); > @@ -1455,6 +1451,11 @@ static int exec_binprm(struct linux_binprm *bprm) > ptrace_event(PTRACE_EVENT_EXEC, old_vpid); > current->did_exec = 1; > proc_exec_connector(current); > + > + if (bprm->file) { > + allow_write_access(bprm->file); > + fput(bprm->file); > + }
Why not keep the bprm->file = NULL assignment? Seems reasonable to keep that just to be avoid use-after-free accidents.
-Kees
> } > > return ret; > -- > 1.5.5.1 >
-- Kees Cook Chrome OS Security
| |