Re: [PATCH v4 3/3] exec: Add support for 64 byte 'tsk->comm_ext'

[Bhupesh Sharma <bhsharma@igalia.com>]

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@mail.gmail.com/
[2]. https://lore.kernel.org/lkml/CALOAHbB51b-reG6+ypr43sBJ-QpQhF39r5WPjuEp5rgabgRmoA@mail.gmail.com/

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...
>