Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Incorrect argument passed to `strscpy_pad` in Smack LSM causes an out-of-bounds read, leaking `struct task_struct` heap memory to the kernel log. - [High] Incorrect argument passed to `copy_task_comm` in the BPF test module causes an invalid pointer dereference and kernel panic. --
commit a226c4fbdd68ce01686ac76a75507a8ad81e5825 Author: André Almeida <[email protected]> Subject: prctl: Add support for long user thread names This commit adds support for getting and setting long user thread names with pr_set_ext_name and pr_get_ext_name. [ ... ] > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -138,6 +138,8 @@ smk_bu_tsk_to_obj(struct task_struct *tsk, const struct > task_smack *tsp, > char acc[SMK_NUM_ACCESS_TYPE + 1]; > char comm[TASK_COMM_LEN]; > > + strscpy_pad(comm, tsk); ^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This isn't a bug introduced by this patch, but by an earlier patch in the reviewed patchset. It remains unfixed in the final state of the series. Does this code pass the correct argument to strscpy_pad()? The strscpy_pad() call implicitly converts struct task_struct *tsk to const char *, causing the first 16 bytes of the task_struct to be copied into the comm buffer and subsequently logged via pr_info(). Could this cause an out-of-bounds read, leaking struct task_struct heap memory to the kernel log when smack bringup auditing is enabled and a relevant access check fails or is logged? Exposing raw task structures to logs bypasses kernel address space layout randomization and leaks kernel layout data. [ ... ] > diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod-events.h > b/tools/testing/selftests/bpf/test_kmods/bpf_testmod-events.h > --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod-events.h > +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod-events.h > @@ -20,7 +20,7 @@ TRACE_EVENT(bpf_testmod_test_read, > ), > TP_fast_assign( > __entry->pid = task->pid; > - memcpy(__entry->comm, task->comm, TASK_COMM_LEN); > + copy_task_comm(__entry->comm, task->comm, TASK_COMM_LEN); ^^^^^^^^^^ [Severity: High] This isn't a bug introduced by this patch, but by an earlier patch in the reviewed patchset. It remains unfixed in the final state of the series. Are we passing the correct argument type to copy_task_comm() here? The inline function copy_task_comm() expects a struct task_struct * but receives task->comm, which is a string pointer. Inside copy_task_comm(), evaluating tsk->comm treats the string's memory address as a task_struct and adds the structural offset of the comm field, yielding a wildly invalid address. When bpf_testmod_test_read() executes during bpf selftests, won't this cause a wild pointer dereference and kernel panic? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
