| 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
| ||
|
Message-ID: <7839453E-CA06-430A-A198-92EB906F94D9@kernel.org>
Date: Wed, 28 Aug 2024 06:48:39 -0700
From: Kees Cook <kees@...nel.org>
To: Yafang Shao <laoar.shao@...il.com>, Alejandro Colomar <alx@...nel.org>
CC: akpm@...ux-foundation.org, torvalds@...ux-foundation.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 August 28, 2024 6:40:35 AM PDT, Yafang Shao <laoar.shao@...il.com> wrote:
>On Wed, Aug 28, 2024 at 8:58 PM Alejandro Colomar <alx@...nel.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@...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/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@...nel.org>
> Signed-off-by: Alejandro Colomar <alx@...nel.org>
> Signed-off-by: Yafang Shao <laoar.shao@...il.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
Powered by blists - more mailing lists