[PATCH v8 1/8] Get rid of __get_task_comm()
Yafang Shao
laoar.shao at gmail.com
Wed Aug 28 13:40:35 UTC 2024
On Wed, Aug 28, 2024 at 8:58 PM Alejandro Colomar <alx at 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 at 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 at kernel.org>
> > > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> > > Signed-off-by: Yafang Shao <laoar.shao at gmail.com>
> > > Cc: Alexander Viro <viro at zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner at kernel.org>
> > > Cc: Jan Kara <jack at suse.cz>
> > > Cc: Eric Biederman <ebiederm at xmission.com>
> > > Cc: Kees Cook <keescook at chromium.org>
> > > Cc: Alexei Starovoitov <alexei.starovoitov at gmail.com>
> > > Cc: Matus Jokay <matus.jokay at stuba.sk>
> > > Cc: Alejandro Colomar <alx at kernel.org>
> > > Cc: "Serge E. Hallyn" <serge at 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 at kernel.org>
Signed-off-by: Alejandro Colomar <alx at kernel.org>
Signed-off-by: Yafang Shao <laoar.shao at 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))
/* Copied from linux/compiler-gcc.h since we can't include it directly */
#define barrier() __asm__ __volatile__("": : :"memory")
diff --git a/include/linux/string.h b/include/linux/string.h
index 9edace076ddb..060504719904 100644
--- a/include/linux/string.h
+++ b/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)
/**
--
Regards
Yafang
More information about the Linux-security-module-archive
mailing list