Message-ID: <CALOAHbBHkS=J8Bv+XsoWvwdfG7fGFg0eVw9PhOVWVbJ1ebrr1w@mail.gmail.com>
Date: Thu, 29 Aug 2024 14:30:14 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Kees Cook <kees@...nel.org>
Cc: akpm@...ux-foundation.org, torvalds@...ux-foundation.org, alx@...nel.org, 
	justinstitt@...gle.com, ebiederm@...ssion.com, alexei.starovoitov@...il.com, 
	rostedt@...dmis.org, catalin.marinas@....com, 
	penguin-kernel@...ove.sakura.ne.jp, linux-mm@...ck.org, 
	linux-fsdevel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	audit@...r.kernel.org, linux-security-module@...r.kernel.org, 
	selinux@...r.kernel.org, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	dri-devel@...ts.freedesktop.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>, "Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH v8 1/8] Get rid of __get_task_comm()

On Wed, Aug 28, 2024 at 10:04 PM Kees Cook <kees@...nel.org> wrote:
>
>
>
> On August 27, 2024 8:03:14 PM PDT, Yafang Shao <laoar.shao@...il.com> wrote:
> >We want to eliminate the use of __get_task_comm() for the following
> >reasons:
> >
> >- The task_lock() is unnecessary
> >  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
> >
> >- The BUILD_BUG_ON() doesn't add any value
> >  The only requirement is to ensure that the destination buffer is a valid
> >  array.
>
> Sorry, that's not a correct evaluation. See below.
>
> >
> >- Zeroing is not necessary in current use cases
> >  To avoid confusion, we should remove it. Moreover, not zeroing could
> >  potentially make it easier to uncover bugs. If the caller needs a
> >  zero-padded task name, it should be explicitly handled at the call site.
>
> This is also not an appropriate rationale. We don't make the kernel "more buggy" not purpose. ;) See below.
>
> >
> >Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> >Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> >Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> >Suggested-by: Alejandro Colomar <alx@...nel.org>
> >Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> >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>
> >Cc: Alejandro Colomar <alx@...nel.org>
> >Cc: "Serge E. Hallyn" <serge@...lyn.com>
> >---
> > fs/exec.c             | 10 ----------
> > fs/proc/array.c       |  2 +-
> > include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> > kernel/kthread.c      |  2 +-
> > 4 files changed, 28 insertions(+), 18 deletions(-)
> >
> >diff --git a/fs/exec.c b/fs/exec.c
> >index 50e76cc633c4..8a23171bc3c3 100644
> >--- a/fs/exec.c
> >+++ b/fs/exec.c
> >@@ -1264,16 +1264,6 @@ static int unshare_sighand(struct task_struct *me)
> >       return 0;
> > }
> >
> >-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);
> >-
> > /*
> >  * These functions flushes out all traces of the currently running executable
> >  * so that a new one can be started
> >diff --git a/fs/proc/array.c b/fs/proc/array.c
> >index 34a47fb0c57f..55ed3510d2bb 100644
> >--- a/fs/proc/array.c
> >+++ b/fs/proc/array.c
> >@@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
> >       else if (p->flags & PF_KTHREAD)
> >               get_kthread_comm(tcomm, sizeof(tcomm), p);
> >       else
> >-              __get_task_comm(tcomm, sizeof(tcomm), p);
> >+              get_task_comm(tcomm, p);
> >
> >       if (escape)
> >               seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
> >diff --git a/include/linux/sched.h b/include/linux/sched.h
> >index f8d150343d42..c40b95a79d80 100644
> >--- a/include/linux/sched.h
> >+++ b/include/linux/sched.h
> >@@ -1096,9 +1096,12 @@ struct task_struct {
> >       /*
> >        * executable name, excluding path.
> >        *
> >-       * - normally initialized setup_new_exec()
> >-       * - access it with [gs]et_task_comm()
> >-       * - lock it with task_lock()
> >+       * - normally initialized begin_new_exec()
> >+       * - set it with set_task_comm()
> >+       *   - strscpy_pad() to ensure it is always NUL-terminated and
> >+       *     zero-padded
> >+       *   - task_lock() to ensure the operation is atomic and the name is
> >+       *     fully updated.
> >        */
> >       char                            comm[TASK_COMM_LEN];
> >
> >@@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> >       __set_task_comm(tsk, from, false);
> > }
> >
> >-extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> >+/*
> >+ * - Why not use task_lock()?
> >+ *   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 and zero-padded. Therefore the race condition between
> >+ *   reader and writer is not an issue.
> >+ *
> >+ * - Why not use strscpy_pad()?
> >+ *   While strscpy_pad() prevents writing garbage past the NUL terminator, which
> >+ *   is useful when using the task name as a key in a hash map, most use cases
> >+ *   don't require this. Zero-padding might confuse users if it’s unnecessary,
> >+ *   and not zeroing might even make it easier to expose bugs. If you need a
> >+ *   zero-padded task name, please handle that explicitly at the call site.
>
> I really don't like this part of the change. You don't know that existing callers don't depend on the padding. Please invert this logic: get_task_comm() must use strscpy_pad(). Calls NOT wanting padding can call strscpy() themselves.
>
> >+ *
> >+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
>
> This doesn't need checking here; strscpy() will already do that.
>
> >+ */
> > #define get_task_comm(buf, tsk) ({                    \
> >-      BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
>
> Also, please leave the TASK_COMM_LEN test so that destination buffers continue to be the correct size: current callers do not perform any return value analysis, so they cannot accidentally start having situations where the destination string might be truncated. Again, anyone wanting to avoid that restriction can use strscpy() directly and check the return value.

Hello Kees,

Thanks for your input.

Alejandro has addressed all the other changes except for the removal
of BUILD_BUG_ON(). I have a question regarding this: if we're using it
to avoid truncation, why not write it like this?

    BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);

This way, it ensures that the size is at least as large as TASK_COMM_LEN.

--
Regards
Yafang

Powered by blists - more mailing lists