Re: [PATCH v6 5/6] prctl: Add support for long user thread names

[sashiko-bot@kernel.org]

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 <andrealmeid@igalia.com>
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/20260910-tonyk-long_name-v6-0-d70afbf194c5@igalia.com?part=5