Message-ID: <1bc43d6c-2650-0670-8c2a-25e8d36cfb7c@igalia.com>
Date: Mon, 26 May 2025 16:43:52 +0530
From: Bhupesh Sharma <bhsharma@...lia.com>
To: Kees Cook <kees@...nel.org>
Cc: Bhupesh <bhupesh@...lia.com>, 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,

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.

Thanks,
Bhupesh

Powered by blists - more mailing lists