exec: Make sure task->comm is always NUL-terminated

Using strscpy() meant that the final character in task->comm may be
non-NUL for a moment before the "string too long" truncation happens.

Instead of adding a new use of the ambiguous strncpy(), we'd want to
use memtostr_pad() which enforces being able to check at compile time
that sizes are sensible, but this requires being able to see string
buffer lengths. Instead of trying to inline __set_task_comm() (which
needs to call trace and perf functions), just open-code it. But to
make sure we're always safe, add compile-time checking like we already
do for get_task_comm().

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Kees Cook <kees@kernel.org>

diff --git a/fs/exec.c b/fs/exec.c
index e0435b31a811af1bca5f18dac5382a4f1e271930..5f16500ac3253d08e811fd678162618b0422d14c 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1200,16 +1200,16 @@ char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
 EXPORT_SYMBOL_GPL(__get_task_comm);
 
 /*
- * These functions flushes out all traces of the currently running executable
- * so that a new one can be started
+ * This is unlocked -- the string will always be NUL-terminated, but
+ * may show overlapping contents if racing concurrent reads.
  */
-
 void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
 {
-       task_lock(tsk);
+       size_t len = min(strlen(buf), sizeof(tsk->comm) - 1);
+
        trace_task_rename(tsk, buf);
-       strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
-       task_unlock(tsk);
+       memcpy(tsk->comm, buf, len);
+       memset(&tsk->comm[len], 0, sizeof(tsk->comm) - len);
        perf_event_comm(tsk, exec);
 }
 

diff --git a/include/linux/sched.h b/include/linux/sched.h
index e6ee4258169a0e17838b4d05ce38cac28056a95f..ac9f429ddc178ee156062005ef847fffdf3714ae 100644 (file)
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1932,11 +1932,10 @@ static inline void kick_process(struct task_struct *tsk) { }
 #endif
 
 extern void __set_task_comm(struct task_struct *tsk, const char *from, bool exec);
-
-static inline void set_task_comm(struct task_struct *tsk, const char *from)
-{
-       __set_task_comm(tsk, from, false);
-}
+#define set_task_comm(tsk, from) ({                    \
+       BUILD_BUG_ON(sizeof(from) != TASK_COMM_LEN);    \
+       __set_task_comm(tsk, from, false);              \
+})
 
 extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
 #define get_task_comm(buf, tsk) ({                     \

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index a38f36b68060410cd0d326ff574aee7278cf2173..5d0928f37471e5334731f75fac3e8ca56802c289 100644 (file)
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -634,7 +634,7 @@ static int io_wq_worker(void *data)
        struct io_wq_acct *acct = io_wq_get_acct(worker);
        struct io_wq *wq = worker->wq;
        bool exit_mask = false, last_timeout = false;
-       char buf[TASK_COMM_LEN];
+       char buf[TASK_COMM_LEN] = {};
 
        set_mask_bits(&worker->flags, 0,
                      BIT(IO_WORKER_F_UP) | BIT(IO_WORKER_F_RUNNING));

diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index a26593979887f30923496632bbdcc1fa6bb2d28f..90011f06c7fbf1e4de8db91ca62225cd3c975354 100644 (file)
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -271,7 +271,7 @@ static int io_sq_thread(void *data)
        struct io_ring_ctx *ctx;
        struct rusage start;
        unsigned long timeout = 0;
-       char buf[TASK_COMM_LEN];
+       char buf[TASK_COMM_LEN] = {};
        DEFINE_WAIT(wait);
 
        /* offload context creation failed, just exit */

diff --git a/kernel/kthread.c b/kernel/kthread.c
index db4ceb0f503cca575efc5a8fc1350aead0efdff6..162d558117447c6bdbd45c60eb6ebc48560c5084 100644 (file)
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -736,10 +736,11 @@ EXPORT_SYMBOL(kthread_stop_put);
 
 int kthreadd(void *unused)
 {
+       static const char comm[TASK_COMM_LEN] = "kthreadd";
        struct task_struct *tsk = current;
 
        /* Setup a clean context for our children to inherit. */
-       set_task_comm(tsk, "kthreadd");
+       set_task_comm(tsk, comm);
        ignore_signals(tsk);
        set_cpus_allowed_ptr(tsk, housekeeping_cpumask(HK_TYPE_KTHREAD));
        set_mems_allowed(node_states[N_MEMORY]);