Commit 3a3f61ce authored by Kees Cook's avatar Kees Cook
Browse files

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: default avatarLinus Torvalds <torvalds@linux-foundation.org>
Suggested-by: default avatar"Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: default avatarKees Cook <kees@kernel.org>
parent fa1bdca9

fs/exec.c

+6 −6
Original line number Diff line number Diff line
@@ -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);

}

include/linux/sched.h

+4 −5
Original line number Diff line number Diff line
@@ -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) ({			\

io_uring/io-wq.c

+1 −1
Original line number Diff line number Diff line
@@ -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));

io_uring/sqpoll.c

+1 −1
Original line number Diff line number Diff line
@@ -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 */

kernel/kthread.c

+2 −1
Original line number Diff line number Diff line
@@ -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]);