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 <[email protected]> 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/[email protected]?part=4
