Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Kees Cook <kees@kernel.org>]

On August 28, 2024 6:40:35 AM PDT, Yafang Shao <laoar.shao@gmail.com> wrote:
>On Wed, Aug 28, 2024 at 8:58 PM Alejandro Colomar <alx@kernel.org> wrote:
>>
>> On Wed, Aug 28, 2024 at 12:15:40PM GMT, Alejandro Colomar wrote:
>> > Hi Yafang,
>> >
>> > On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao 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.
>> > >
>> > > - 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 [0]
>> > > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
>> > > Suggested-by: Alejandro Colomar <alx@kernel.org>
>> > > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
>> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> > > Cc: Christian Brauner <brauner@kernel.org>
>> > > Cc: Jan Kara <jack@suse.cz>
>> > > Cc: Eric Biederman <ebiederm@xmission.com>
>> > > Cc: Kees Cook <keescook@chromium.org>
>> > > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> > > Cc: Matus Jokay <matus.jokay@stuba.sk>
>> > > 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(-)
>> > >
>> >
>> > [...]
>> >
>> > > 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
>> >
>> > [...]
>> >
>> > > @@ -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);
>> > > +/*
>> >
>> > [...]
>> >
>> > > + * - 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 <alx@kernel.org>
>    Signed-off-by: Alejandro Colomar <alx@kernel.org>
>    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
>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))

Uh, but why? strscpy() copies bytes, not array elements. Using sizeof() is already correct and using ARRAY_SIZE() could lead to unexpectedly small counts (in admittedly odd situations).

What is the problem you're trying to solve here?

-Kees

-- 
Kees Cook