On Wed, Aug 28, 2024 at 6:15 PM Alejandro Colomar [off-list ref] wrote:

Hi Yafang,

On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:

quoted

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.

- 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.

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]
Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ (local)
Suggested-by: Alejandro Colomar <alx@kernel.org>
Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq (local)
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>
Cc: Alexei Starovoitov <redacted>
Cc: Matus Jokay <redacted>
Cc: Alejandro Colomar <alx@kernel.org>
Cc: "Serge E. Hallyn" <serge@hallyn.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(-)

[...]

quoted

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

[...]

quoted

@@ -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);
+/*

[...]

quoted

+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
+ */
 #define get_task_comm(buf, tsk) ({                   \
-     BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
-     __get_task_comm(buf, sizeof(buf), tsk);         \
+     strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));     \

I see that there's a two-argument macro

        #define strscpy(dst, src)       sized_strscpy(dst, src, sizeof(dst))

This macro is defined in arch/um/include/shared/user.h, which is not
used outside
the arch/um/ directory.
This marco should be addressed.

which is used in patch 2/8

The strscpy() function used in this series is defined in
include/linux/string.h, which already checks whether the input is an
array:

#define __strscpy0(dst, src, ...)       \
        sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
#define __strscpy1(dst, src, size)      sized_strscpy(dst, src, size)

#define __strscpy_pad0(dst, src, ...)   \
        sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
#define __strscpy_pad1(dst, src, size)  sized_strscpy_pad(dst, src, size)

        diff --git a/kernel/auditsc.c b/kernel/auditsc.c
        index 6f0d6fb6523f..e4ef5e57dde9 100644
        --- a/kernel/auditsc.c
        +++ b/kernel/auditsc.c
        @@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
                context->target_uid = task_uid(t);
                context->target_sessionid = audit_get_sessionid(t);
                security_task_getsecid_obj(t, &context->target_sid);
        -       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
        +       strscpy(context->target_comm, t->comm);
         }

         /**

I propose modifying that macro to use ARRAY_SIZE() instead of sizeof(),
and then calling that macro here too.  That would not only make sure
that this is an array, but make sure that every call to that macro is an
array.  An if there are macros for similar string functions that reduce
the argument with a usual sizeof(), the same thing could be done to
those too.

I have no preference between using ARRAY_SIZE() or sizeof(dst) +
__must_be_array(dst). However, for consistency, it might be better to
use ARRAY_SIZE().


--
Regards

Yafang