Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] blktrace binary API breakage due to increased payload size for BLK_TN_PROCESS events. - [Medium] Audit logs expose the full 64-byte extended task name, violating the commit message's claim to cap all existing userspace APIs to 16 bytes. - [Medium] The tracepoint `bpf_testmod_test_read` copies the task name without guaranteeing NUL termination, leading to an out-of-bounds read when printing. - [Low] The patch series removed the `get_task_comm` macro but failed to update its callers in the Smack LSM, resulting in an implicit function declaration and a compilation error. --
Hi, I have some questions about this patch. commit 4fa4b0c6a1f339946bd26a6d2c55d7f5cfb4c1f3 Author: André Almeida <[email protected]> Subject: sched: Extend task command name with TASK_COMM_EXT_LEN The commit increases the task command name from 16 to 64 bytes. It aims to support long names for userspace threads while capping existing userspace APIs to TASK_COMM_LEN to avoid buffer overflows. > diff --git a/include/linux/sched.h b/include/linux/sched.h > index c7cdbb3e4aa68..c41d61e9babf9 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]; > > struct nameidata *nameidata; [Severity: Critical] With the struct task_struct comm field expanding to 64 bytes, does this break the blktrace binary ABI? In kernel/trace/blktrace.c:trace_note_tsk(): trace_note(bt, tsk->pid, BLK_TN_PROCESS, tsk->comm, sizeof(tsk->comm), 0); This will now write 64 bytes to the UAPI trace buffer instead of 16 bytes, which can cause parsing errors or buffer overflows in userspace tools like blkparse. [Severity: Medium] Will out-of-bounds reads happen in existing tracepoints that copy directly from task->comm using TASK_COMM_LEN? For example, in tools/testing/selftests/bpf/test_kmods/bpf_testmod-events.h: memcpy(__entry->comm, task->comm, TASK_COMM_LEN); If a task name is 16 bytes or longer, there won't be a NUL terminator in the first 16 bytes, which might cause TP_printk("%s") to read past the 16 bytes into adjacent trace event fields. [ ... ] > -#define get_task_comm(buf, tsk) ({ \ > - BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN); \ > - strscpy_pad(buf, (tsk)->comm); \ > - buf; \ > -}) [Severity: Low] Does removing get_task_comm() break the build if CONFIG_SECURITY_SMACK=y? Callers in security/smack/smack_access.c:smack_log() and security/smack/smack_lsm.c:smk_bu_current() still seem to use this macro. > diff --git a/kernel/audit.c b/kernel/audit.c > index 9412af9144bc9..75bcc0d43c558 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1667,7 +1667,8 @@ static void audit_log_multicast(int group, const char > *op, int err) > audit_put_tty(tty); > audit_log_task_context(ab); /* subj= */ > audit_log_format(ab, " comm="); > - audit_log_untrustedstring(ab, get_task_comm(comm, current)); > + strscpy_pad(comm, current->comm); > + audit_log_untrustedstring(ab, comm); [Severity: Medium] Since comm is declared as char comm[sizeof(current->comm)] at the top of this function, and current->comm is now 64 bytes, does this expose the full 64-byte task name in the audit log? The commit message mentions capping all existing userspace APIs to TASK_COMM_LEN. This same pattern appears in kernel/audit.c:audit_log_task_info(), kernel/auditsc.c:audit_log_task(), and security/lsm_audit.c:dump_common_audit_data(). -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
