Messages in this thread Patch in this message | | | Date | Wed, 5 Feb 2014 14:52:03 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC][PATCH] exec: Fix use after free of tracepoint trace_sched_process_exec |
| |
On 02/04, Linus Torvalds wrote: > > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -239,7 +239,7 @@ static int ____call_usermodehelper(void *data) > > commit_creds(new); > > - retval = do_execve(sub_info->path, > + retval = do_execve(getname_kernel(sub_info->path), > (const char __user *const __user *)sub_info->argv, > (const char __user *const __user *)sub_info->envp);
Great, this naturally duplicates filename unconditionally, and we can kill bprm->tcomm[].
But,
> --- a/include/linux/binfmts.h > +++ b/include/linux/binfmts.h > @@ -37,7 +37,7 @@ struct linux_binprm { > int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ > unsigned int per_clear; /* bits to clear in current->personality */ > int argc, envc; > - const char * filename; /* Name of binary as seen by procps */ > + struct filename *filename; /* Name of binary as seen by procps */
Do we really need this change? If not (afaics), the patch can be much simpler, see below...
> -void free_bprm(struct linux_binprm *bprm) > +static void free_bprm(struct linux_binprm *bprm) > { > free_arg_pages(bprm); > if (bprm->cred) { > @@ -1174,15 +1179,17 @@ void free_bprm(struct linux_binprm *bprm) > fput(bprm->file); > } > /* If a binfmt changed the interp, free it. */ > - if (bprm->interp != bprm->filename) > + if (bprm->interp != bprm->filename->name) > kfree(bprm->interp); > + if (bprm->filename) > + putname(bprm->filename);
Even if we actually need to turn bprm->filename into "struct filename" this free_bprm()->putname() only complicates the code, unless I missed something. The caller, do_execve(), can do putname() unconditionally and avoid if/NULL games.
IOW, doesn't the change below (on top of your patch) obviously makes sense or I am totally confused?
Oleg.
--- x/fs/exec.c +++ x/fs/exec.c @@ -1183,8 +1183,6 @@ static void free_bprm(struct linux_binpr /* If a binfmt changed the interp, free it. */ if (bprm->interp != bprm->filename->name) kfree(bprm->interp); - if (bprm->filename) - putname(bprm->filename); kfree(bprm); } @@ -1478,9 +1476,6 @@ static int do_execve_common(struct filen if (!bprm) goto out_files; - bprm->filename = filename; - bprm->interp = filename->name; - retval = prepare_bprm_creds(bprm); if (retval) goto out_free; @@ -1496,6 +1491,8 @@ static int do_execve_common(struct filen sched_exec(); bprm->file = file; + bprm->filename = filename; + bprm->interp = filename->name; retval = bprm_mm_init(bprm); if (retval) @@ -1538,7 +1535,7 @@ static int do_execve_common(struct filen free_bprm(bprm); if (displaced) put_files_struct(displaced); - return retval; + goto out_ret; out: if (bprm->mm) { @@ -1552,14 +1549,12 @@ out_unmark: out_free: free_bprm(bprm); - filename = NULL; out_files: if (displaced) reset_files_struct(displaced); out_ret: - if (filename) - putname(filename); + putname(filename); return retval; }
| |