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@xxxxxxxxxxxxxx/
[2]. https://lore.kernel.org/all/202505231346.52F291C54@keescook/
Thanks.
