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@xxxxxxxxxxxxxxxxxxxx> > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@xxxxxxxxxxxxxx [0] > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@xxxxxxxxxxxxxx/ > Suggested-by: Alejandro Colomar <alx@xxxxxxxxxx> > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx> > Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx> > Cc: Christian Brauner <brauner@xxxxxxxxxx> > Cc: Jan Kara <jack@xxxxxxx> > Cc: Eric Biederman <ebiederm@xxxxxxxxxxxx> > Cc: Kees Cook <keescook@xxxxxxxxxxxx> > Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> > Cc: Matus Jokay <matus.jokay@xxxxxxxx> > Cc: Alejandro Colomar <alx@xxxxxxxxxx> > Cc: "Serge E. Hallyn" <serge@xxxxxxxxxx> > --- > 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); } /** 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. Have a lovley day! Alex > + buf; \ > }) > > #ifdef CONFIG_SMP > diff --git a/kernel/kthread.c b/kernel/kthread.c > index f7be976ff88a..7d001d033cf9 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk) > struct kthread *kthread = to_kthread(tsk); > > if (!kthread || !kthread->full_name) { > - __get_task_comm(buf, buf_size, tsk); > + strscpy(buf, tsk->comm, buf_size); > return; > } > > -- > 2.43.5 > -- <https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
