Hi Kees,
Thanks for the review.
On 5/23/25 9:18 AM, Kees Cook wrote:
On Wed, May 21, 2025 at 11:53:37AM +0530, Bhupesh wrote:
Historically due to the 16-byte length of TASK_COMM_LEN, the
users of 'tsk->comm' are restricted to use a fixed-size target
buffer also of TASK_COMM_LEN for 'memcpy()' like use-cases.
To fix the same, Linus suggested in [1] that we can add the
following union inside 'task_struct':
union {
char comm[TASK_COMM_LEN];
char comm_ext[TASK_COMM_EXT_LEN];
};
I remain unconvinced that this is at all safe. With the existing
memcpy() and so many places using %s and task->comm, this feels very
very risky to me.
Can we just make it separate, instead of a union? Then we don't have to
touch comm at all.
I understand your apprehensions, but I think we have covered _almost_
all the existing use-cases as of now:
1. memcpy() users: Handled by [PATCH 2/3] of this series, where we
identify existing users using the following search
pattern:
$ git grep 'memcpy.*->comm\>'
2. %s usage: I checked this at multiple places and can confirm that %s
usage to print out 'tsk->comm' (as a string), get the longer
new "extended comm".
3. users who do 'sizeof(->comm)' will continue to get the old value
because of the union.
The problem with having two separate comms: tsk->comm and tsk->ext_comm,
instead of a union is two fold:
(a). If we keep two separate statically allocated comms: tsk->comm and
tsk->ext_comm in struct task_struct, we need to basically keep
supporting backward compatibility / ABI via tsk->comm and ask new
user-land users to move to tsk->ext_comm.
(b). If we keep one statically allocated comm: tsk->comm and one dynamically allocated tsk->ext_comm in struct task_struct, then we have the problem of allocating the tsk->ext_comm which _may_ be in the exec() hot path.
I think the discussion between Linus and Yafang (see [1]), was more towards avoiding the approach in 3(a).
Also we discussed the 3(b) approach, during the review of v2 of this series, where there was a apprehensions around: adding another field to store the task name and allocating tsk->ext_comm dynamically in the exec() hot path (see [2]).
[1]. https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@xxxxxxxxxxxxxx/
[2]. https://lore.kernel.org/lkml/CALOAHbB51b-reG6+ypr43sBJ-QpQhF39r5WPjuEp5rgabgRmoA@xxxxxxxxxxxxxx/
Please let me know your views.
Thanks,
Bhupesh
and then modify '__set_task_comm()' to pass 'tsk->comm_ext'
to the existing users.
We can use set_task_comm() to set both still...
