Re: [PATCH 1/6] fs/exec: Drop task_lock() inside __get_task_comm()
From: Eric W. Biederman <hidden>
Date: 2024-06-02 04:08:52
Also in:
bpf, linux-fsdevel, linux-mm, linux-security-module, selinux
Yafang Shao [off-list ref] writes:
Quoted from Linus [0]: Since user space can randomly change their names anyway, using locking was always wrong for readers (for writers it probably does make sense to have some lock - although practically speaking nobody cares there either, but at least for a writer some kind of race could have long-term mixed results
Ugh.
Ick.
This code is buggy.
I won't argue that Linus is wrong, about removing the
task_lock.
Unfortunately strscpy_pad does not work properly with the
task_lock removed, and buf_size larger that TASK_COMM_LEN.
There is a race that will allow reading past the end
of tsk->comm, if we read while tsk->common is being
updated.
So __get_task_comm needs to look something like:
char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
{
size_t len = buf_size;
if (len > TASK_COMM_LEN)
len = TASK_COMM_LEN;
memcpy(buf, tsk->comm, len);
buf[len -1] = '\0';
return buf;
}
What shows up in buf past the '\0' is not guaranteed in the above
version but I would be surprised if anyone cares.
If people do care the code can do something like:
char *last = strchr(buf);
memset(last, '\0', buf_size - (last - buf));
To zero everything in the buffer past the first '\0' byte.
Eric
quoted hunk
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com (local) [0] Signed-off-by: Yafang Shao <redacted> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <redacted> Cc: Kees Cook <redacted> --- fs/exec.c | 7 +++++-- include/linux/sched.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)diff --git a/fs/exec.c b/fs/exec.c index b3c40fbb325f..b43992d35a8a 100644 --- a/fs/exec.c +++ b/fs/exec.c@@ -1227,12 +1227,15 @@ static int unshare_sighand(struct task_struct *me) return 0; } +/* + * User space can randomly change their names anyway, so locking for readers + * doesn't make sense. For writers, locking is probably necessary, as a race + * condition could lead to long-term mixed results. + */ char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) { - task_lock(tsk); /* Always NUL terminated and zero-padded */ strscpy_pad(buf, tsk->comm, buf_size); - task_unlock(tsk); return buf; } EXPORT_SYMBOL_GPL(__get_task_comm);diff --git a/include/linux/sched.h b/include/linux/sched.h index c75fd46506df..56a927393a38 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h@@ -1083,7 +1083,7 @@ struct task_struct { * * - normally initialized setup_new_exec() * - access it with [gs]et_task_comm() - * - lock it with task_lock() + * - lock it with task_lock() for writing */ char comm[TASK_COMM_LEN];