netdev - [PATCH v4 01/11] fs/exec: Drop task_lock() inside __get_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240628090517.17994-1-laoar.shao@gmail.com>
Date: Fri, 28 Jun 2024 17:05:07 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: torvalds@...ux-foundation.org,
	laoar.shao@...il.com
Cc: akpm@...ux-foundation.org,
	alexei.starovoitov@...il.com,
	audit@...r.kernel.org,
	bpf@...r.kernel.org,
	catalin.marinas@....com,
	dri-devel@...ts.freedesktop.org,
	ebiederm@...ssion.com,
	linux-fsdevel@...r.kernel.org,
	linux-mm@...ck.org,
	linux-security-module@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	penguin-kernel@...ove.sakura.ne.jp,
	rostedt@...dmis.org,
	selinux@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>,
	Jan Kara <jack@...e.cz>,
	Kees Cook <keescook@...omium.org>,
	Matus Jokay <matus.jokay@...ba.sk>
Subject: [PATCH v4 01/11] fs/exec: Drop task_lock() inside __get_task_comm()

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

Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
Signed-off-by: Yafang Shao <laoar.shao@...il.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>
Cc: Christian Brauner <brauner@...nel.org>
Cc: Jan Kara <jack@...e.cz>
Cc: Eric Biederman <ebiederm@...ssion.com>
Cc: Kees Cook <keescook@...omium.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Matus Jokay <matus.jokay@...ba.sk>
---
 fs/exec.c             | 10 ++++++++--
 include/linux/sched.h |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 40073142288f..fa6b61c79df8 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1238,12 +1238,18 @@ 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.
+ * The strscpy_pad() in __set_task_comm() can ensure that the task comm is
+ * always NUL-terminated. Therefore the race condition between reader and writer
+ * is not an issue.
+ */
 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 61591ac6eab6..95888d1da49e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1086,9 +1086,9 @@ struct task_struct {
 	/*
 	 * executable name, excluding path.
 	 *
-	 * - normally initialized setup_new_exec()
+	 * - normally initialized begin_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];
 
-- 
2.43.5