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

[Alejandro Colomar]

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

	 /**

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/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240828/27a20128/attachment.sig>