exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case

Zbigniew mentioned at Linux Plumber's that systemd is interested in
switching to execveat() for service execution, but can't, because the
contents of /proc/pid/comm are the file descriptor which was used,
instead of the path to the binary[1]. This makes the output of tools like
top and ps useless, especially in a world where most fds are opened
CLOEXEC so the number is truly meaningless.

When the filename passed in is empty (e.g. with AT_EMPTY_PATH), use the
dentry's filename for "comm" instead of using the useless numeral from
the synthetic fdpath construction. This way the actual exec machinery
is unchanged, but cosmetically the comm looks reasonable to admins
investigating things.

Instead of adding TASK_COMM_LEN more bytes to bprm, use one of the unused
flag bits to indicate that we need to set "comm" from the dentry.

Suggested-by: Zbigniew Jędrzejewski-Szmek <[email protected]>
Suggested-by: Tycho Andersen <[email protected]>
Suggested-by: Al Viro <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Link: https://github.com/uapi-group/kernel-features#set-comm-field-before-exec
Reviewed-by: Aleksa Sarai <[email protected]>
Tested-by: Zbigniew Jędrzejewski-Szmek <[email protected]>
Signed-off-by: Kees Cook <[email protected]>

diff --git a/fs/exec.c b/fs/exec.c
index 5f16500ac3253d08e811fd678162618b0422d14c..1843366be6ffd8f40b7fcbadf840e672aeaa8e12 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1347,7 +1347,28 @@ int begin_new_exec(struct linux_binprm * bprm)
                set_dumpable(current->mm, SUID_DUMP_USER);
 
        perf_event_exec();
-       __set_task_comm(me, kbasename(bprm->filename), true);
+
+       /*
+        * If the original filename was empty, alloc_bprm() made up a path
+        * that will probably not be useful to admins running ps or similar.
+        * Let's fix it up to be something reasonable.
+        */
+       if (bprm->comm_from_dentry) {
+               /*
+                * Hold RCU lock to keep the name from being freed behind our back.
+                * Use acquire semantics to make sure the terminating NUL from
+                * __d_alloc() is seen.
+                *
+                * Note, we're deliberately sloppy here. We don't need to care about
+                * detecting a concurrent rename and just want a terminated name.
+                */
+               rcu_read_lock();
+               __set_task_comm(me, smp_load_acquire(&bprm->file->f_path.dentry->d_name.name),
+                               true);
+               rcu_read_unlock();
+       } else {
+               __set_task_comm(me, kbasename(bprm->filename), true);
+       }
 
        /* An exec changes our domain. We are no longer part of the thread
           group */
@@ -1521,11 +1542,13 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
        if (fd == AT_FDCWD || filename->name[0] == '/') {
                bprm->filename = filename->name;
        } else {
-               if (filename->name[0] == '\0')
+               if (filename->name[0] == '\0') {
                        bprm->fdpath = kasprintf(GFP_KERNEL, "/dev/fd/%d", fd);
-               else
+                       bprm->comm_from_dentry = 1;
+               } else {
                        bprm->fdpath = kasprintf(GFP_KERNEL, "/dev/fd/%d/%s",
                                                  fd, filename->name);
+               }
                if (!bprm->fdpath)
                        goto out_free;
 

diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index e6c00e860951ae68f9ccda70992d2a43a7f13a7d..3305c849abd66a4ddfd8d7c83ea6ff4dee2b75ad 100644 (file)
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -42,7 +42,9 @@ struct linux_binprm {
                 * Set when errors can no longer be returned to the
                 * original userspace.
                 */
-               point_of_no_return:1;
+               point_of_no_return:1,
+               /* Set when "comm" must come from the dentry. */
+               comm_from_dentry:1;
        struct file *executable; /* Executable to pass to the interpreter */
        struct file *interpreter;
        struct file *file;