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

[Bhupesh Sharma <bhsharma@igalia.com>]

On 5/26/25 4:43 PM, Bhupesh Sharma wrote:
> Hi Kees,
>
> On 5/24/25 2:25 AM, Kees Cook wrote:
>> On Fri, May 23, 2025 at 06:01:41PM +0530, Bhupesh Sharma wrote:
>>> 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".
>> As an example of why I don't like this union is that this is now lying
>> to the compiler. e.g. a %s of an object with a known size (sizeof(comm))
>> may now run off the end of comm without finding a %NUL character... this
>> is "safe" in the sense that the "extended comm" is %NUL terminated, but
>> it makes the string length ambiguous for the compiler (and any
>> associated security hardening).
>
> Right.
>
>>
>>> 3. users who do 'sizeof(->comm)' will continue to get the old value 
>>> because
>>> of the union.
>> Right -- this is exactly where I think it can get very very wrong,
>> leaving things unterminated.
>>
>>> 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]).
>> Right -- I agree we need them statically allocated. But I think a union
>> is going to be really error-prone.
>>
>> How about this: rename task->comm to something else (task->comm_str?),
>> increase its size and then add ABI-keeping wrappers for everything that
>> _must_ have the old length.
>>
>> Doing this guarantees we won't miss anything (since "comm" got renamed),
>> and during the refactoring all the places where the old length is 
>> required
>> will be glaringly obvious. (i.e. it will be harder to make mistakes
>> about leaving things unterminated.)
>>
>
> Ok, I got your point. Let me explore then how best a ABI-keeping 
> wrapper can be introduced.
> I am thinking of something like:
>
> abi_wrapper_get_task_comm {
>
>     if (requested_comm_length <= 16)
>         return 16byte comm with NUL terminator; // old comm (16-bytes)
>     else
>         return 64byte comm with NUL terminator; // extended comm 
> (64-bytes)
>     ....
> }
>
> Please let me know if this looks better. Accordingly I will start with 
> v5 changes.

Hi Everyone, sorry for the delay but I wanted the revive this discussion 
after the -rc1 and my PTO.

I am looking for suggestions on how to implement v5 for this series. 
Here is some background of the version (and related discussions so far):

In the v4, the implementation for tsk->comm handling (for supporting 
long 64byte task names) looked at handling the possible use-cases as 
follows:

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 above points were taken to address the points discussed earlier 
between Linus and Yafang (see [1])

As Kees, suggested in the v4 review (see [2]):
1. Let's rename task->comm to something else (task->comm_str?) and 
increase its size, and

2. Then add ABI-keeping wrappers for everything that  _must_ have the 
old length.

I am thinking of implementing it with something like:

abi_wrapper_get_task_comm {

     if (requested_comm_length <= 16)
         return 16byte comm with NUL terminator; // old comm (16-bytes)
     else
         return 64byte comm with NUL terminator; // extended comm 
(64-bytes)
     ....
}

Kindly let me know your views on the above approach(es).

[1]. 
https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
[2]. https://lore.kernel.org/all/202505231346.52F291C54@keescook/

Thanks.