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

quoted

On Wed, Aug 28, 2024 at 12:15:40PM GMT, Alejandro Colomar wrote:

quoted

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

which is used in patch 2/8

      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);
       }

       /**

Ahh, the actual generic definition is in <include/linux/string.h>.
You could do

        diff --git i/include/linux/string.h w/include/linux/string.h
        index 9edace076ddb..060504719904 100644
        --- i/include/linux/string.h
        +++ w/include/linux/string.h
        @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
          * known size.
          */
         #define __strscpy0(dst, src, ...)      \
        -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
        +       sized_strscpy(dst, src, ARRAY_SIZE(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))
        +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
         #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)

         /**

Thank you for your suggestion. How does the following commit log look
to you? Does it meet your expectations?

   string: Use ARRAY_SIZE() in strscpy()

   We can use ARRAY_SIZE() instead to clarify that they are regular characters.

   Co-developed-by: Alejandro Colomar [off-list ref]
   Signed-off-by: Alejandro Colomar [off-list ref]
   Signed-off-by: Yafang Shao [off-list ref]

diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index bbab79c0c074..07216996e3a9 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h

@@ -14,7 +14,7 @@
 * copying too much infrastructure for my taste, so userspace files
 * get less checking than kernel files.
 */
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))

/* This is to get size_t and NULL */
#ifndef __UM_HOST__

@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,

const char *prefix_str,
extern int in_aton(char *str);
extern size_t strlcat(char *, const char *, size_t);
extern size_t sized_strscpy(char *, const char *, size_t);
-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))