Message-ID: <a7c323fe-6d11-4a21-a203-bd60acbfd831@igalia.com>
Date: Fri, 23 May 2025 18:01:41 +0530
From: Bhupesh Sharma <bhsharma@...lia.com>
To: Kees Cook <kees@...nel.org>, Bhupesh <bhupesh@...lia.com>
Cc: akpm@...ux-foundation.org, kernel-dev@...lia.com,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
 linux-perf-users@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-mm@...ck.org, oliver.sang@...el.com, lkp@...el.com,
 laoar.shao@...il.com, pmladek@...e.com, rostedt@...dmis.org,
 mathieu.desnoyers@...icios.com, arnaldo.melo@...il.com,
 alexei.starovoitov@...il.com, andrii.nakryiko@...il.com,
 mirq-linux@...e.qmqm.pl, peterz@...radead.org, willy@...radead.org,
 david@...hat.com, viro@...iv.linux.org.uk, ebiederm@...ssion.com,
 brauner@...nel.org, jack@...e.cz, mingo@...hat.com, juri.lelli@...hat.com,
 bsegall@...gle.com, mgorman@...e.de, vschneid@...hat.com,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/3] exec: Add support for 64 byte 'tsk->comm_ext'

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

Powered by blists - more mailing lists