execve: use 'struct filename *' for executable name passing

This changes 'do_execve()' to get the executable name as a 'struct
filename', and to free it when it is done.  This is what the normal
users want, and it simplifies and streamlines their error handling.

The controlled lifetime of the executable name also fixes a
use-after-free problem with the trace_sched_process_exec tracepoint: the
lifetime of the passed-in string for kernel users was not at all
obvious, and the user-mode helper code used UMH_WAIT_EXEC to serialize
the pathname allocation lifetime with the execve() having finished,
which in turn meant that the trace point that happened after
mm_release() of the old process VM ended up using already free'd memory.

To solve the kernel string lifetime issue, this simply introduces
"getname_kernel()" that works like the normal user-space getname()
function, except with the source coming from kernel memory.

As Oleg points out, this also means that we could drop the tcomm[] array
from 'struct linux_binprm', since the pathname lifetime now covers
setup_new_exec().  That would be a separate cleanup.

Reported-by: Igor Zhbanov <i.zhbanov@samsung.com>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

1 files changed, 21 insertions, 24 deletions

diff --git a/fs/exec.c b/fs/exec.c
index e1529b4c79b1..3d78fccdd723 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -748,11 +748,10 @@ EXPORT_SYMBOL(setup_arg_pages);
 
 #endif /* CONFIG_MMU */
 
-struct file *open_exec(const char *name)
+static struct file *do_open_exec(struct filename *name)
 {
         struct file *file;
         int err;
-        struct filename tmp = { .name = name };
         static const struct open_flags open_exec_flags = {
                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
                 .acc_mode = MAY_EXEC | MAY_OPEN,
@@ -760,7 +759,7 @@ struct file *open_exec(const char *name)
                 .lookup_flags = LOOKUP_FOLLOW,
         };
 
-        file = do_filp_open(AT_FDCWD, &tmp, &open_exec_flags);
+        file = do_filp_open(AT_FDCWD, name, &open_exec_flags);
         if (IS_ERR(file))
                 goto out;
 
@@ -784,6 +783,12 @@ exit:
         fput(file);
         return ERR_PTR(err);
 }
+
+struct file *open_exec(const char *name)
+{
+        struct filename tmp = { .name = name };
+        return do_open_exec(&tmp);
+}
 EXPORT_SYMBOL(open_exec);
 
 int kernel_read(struct file *file, loff_t offset,
@@ -1162,7 +1167,7 @@ int prepare_bprm_creds(struct linux_binprm *bprm)
         return -ENOMEM;
 }
 
-void free_bprm(struct linux_binprm *bprm)
+static void free_bprm(struct linux_binprm *bprm)
 {
         free_arg_pages(bprm);
         if (bprm->cred) {
@@ -1432,7 +1437,7 @@ static int exec_binprm(struct linux_binprm *bprm)
 /*
  * sys_execve() executes a new program.
  */
-static int do_execve_common(const char *filename,
+static int do_execve_common(struct filename *filename,
                                 struct user_arg_ptr argv,
                                 struct user_arg_ptr envp)
 {
@@ -1441,6 +1446,9 @@ static int do_execve_common(const char *filename,
         struct files_struct *displaced;
         int retval;
 
+        if (IS_ERR(filename))
+                return PTR_ERR(filename);
+
         /*
          * We move the actual failure in case of RLIMIT_NPROC excess from
          * set*uid() to execve() because too many poorly written programs
@@ -1473,7 +1481,7 @@ static int do_execve_common(const char *filename,
         check_unsafe_exec(bprm);
         current->in_execve = 1;
 
-        file = open_exec(filename);
+        file = do_open_exec(filename);
         retval = PTR_ERR(file);
         if (IS_ERR(file))
                 goto out_unmark;
@@ -1481,8 +1489,7 @@ static int do_execve_common(const char *filename,
         sched_exec();
 
         bprm->file = file;
-        bprm->filename = filename;
-        bprm->interp = filename;
+        bprm->filename = bprm->interp = filename->name;
 
         retval = bprm_mm_init(bprm);
         if (retval)
@@ -1523,6 +1530,7 @@ static int do_execve_common(const char *filename,
         acct_update_integrals(current);
         task_numa_free(current);
         free_bprm(bprm);
+        putname(filename);
         if (displaced)
                 put_files_struct(displaced);
         return retval;
@@ -1544,10 +1552,11 @@ out_files:
         if (displaced)
                 reset_files_struct(displaced);
 out_ret:
+        putname(filename);
         return retval;
 }
 
-int do_execve(const char *filename,
+int do_execve(struct filename *filename,
         const char __user *const __user *__argv,
         const char __user *const __user *__envp)
 {
@@ -1557,7 +1566,7 @@ int do_execve(const char *filename,
 }
 
 #ifdef CONFIG_COMPAT
-static int compat_do_execve(const char *filename,
+static int compat_do_execve(struct filename *filename,
         const compat_uptr_t __user *__argv,
         const compat_uptr_t __user *__envp)
 {
@@ -1607,25 +1616,13 @@ SYSCALL_DEFINE3(execve,
                 const char __user *const __user *, argv,
                 const char __user *const __user *, envp)
 {
-        struct filename *path = getname(filename);
-        int error = PTR_ERR(path);
-        if (!IS_ERR(path)) {
-                error = do_execve(path->name, argv, envp);
-                putname(path);
-        }
-        return error;
+        return do_execve(getname(filename), argv, envp);
 }
 #ifdef CONFIG_COMPAT
 asmlinkage long compat_sys_execve(const char __user * filename,
         const compat_uptr_t __user * argv,
         const compat_uptr_t __user * envp)
 {
-        struct filename *path = getname(filename);
-        int error = PTR_ERR(path);
-        if (!IS_ERR(path)) {
-                error = compat_do_execve(path->name, argv, envp);
-                putname(path);
-        }
-        return error;
+        return compat_do_execve(getname(filename), argv, envp);
 }
 #endif