Re: [PATCH v7 4/6] sched: Extend task command name with TASK_COMM_EXT_LEN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Increasing `tsk->comm` to 64 bytes breaks the implicit null-termination API contracts for both `__set_task_comm` and `copy_task_comm`, leading to kernel out-of-bounds reads and trace data corruption.
--

commit 5f2c83bf644707c42f3dee91b932f9dde7986c03
Author: André Almeida <andrealmeid@igalia.com>

sched: Extend task command name with TASK_COMM_EXT_LEN
    
This commit extends the task command name from 16 bytes to 64 bytes by
adding TASK_COMM_EXT_LEN. It caps existing userspace APIs to
TASK_COMM_LEN to avoid buffer overflows while leaving the full extended
name for a new interface.

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index ba20405664246..c4e0104e7ae9b 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
[ ... ]
> @@ -1187,7 +1188,7 @@ struct task_struct {
>  	 * - set it with set_task_comm() to ensure it is always
>  	 *   NUL-terminated and zero-padded
>  	 */
> -	char				comm[TASK_COMM_LEN];
> +	char				comm[TASK_COMM_EXT_LEN];

[Severity: High]
Does this change cause an out-of-bounds read in __set_task_comm()?

__set_task_comm() in fs/exec.c relies on sizeof(tsk->comm) to limit the read
length:

    size_t len = strnlen(buf, sizeof(tsk->comm) - 1);

With tsk->comm now being 64 bytes, strnlen() will try to read up to 63 bytes
from buf. However, the set_task_comm() macro still explicitly permits
passing 16-byte arrays:

    #define set_task_comm(tsk, from) ({ \
        BUILD_BUG_ON(sizeof(from) < TASK_COMM_LEN); \
        __set_task_comm(tsk, from, false); \
    })

If a caller passes a 16-byte array without a null terminator, __set_task_comm()
will now read up to 48 bytes past the end of the array.

[ ... ]
> @@ -2042,7 +2043,7 @@ extern void __set_task_comm(struct task_struct *tsk, const char *from, bool exec
>  static __always_inline void copy_task_comm(char *dst, struct task_struct *tsk, size_t len)
>  {
>  	const char *_src = tsk->comm;
> -	size_t _len = min(len, TASK_COMM_LEN);
> +	size_t _len = min(len, TASK_COMM_EXT_LEN);
>  
>  	if (!_len)
>  		return;

[Severity: High]
Can this cause trace data corruption by omitting the null terminator?

When copy_task_comm() is called from a tracepoint with len=16 and a task name
longer than 15 characters, it will now copy 16 non-null characters into dst
without adding a null terminator.

Previously, tsk->comm was guaranteed to have a null terminator within the
first 16 bytes. Now that the name can be longer, the first 16 bytes might
all be non-null characters.

When the trace ring buffer is later formatted using %s, vsnprintf() will read
past the 16-byte array into adjacent ring buffer fields, causing an
out-of-bounds read and leaking or corrupting trace data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-tonyk-long_name-v7-0-34e1ee1564ae@igalia.com?part=4