linux-kernel - Re: [PATCH v4 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1ff57d4d-6e57-20f1-c3e3-b0f7dfeccaaf@igalia.com>
Date: Fri, 23 May 2025 01:14:05 +0530
From: Bhupesh Sharma <bhsharma@...lia.com>
To: Yafang Shao <laoar.shao@...il.com>, 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, 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, keescook@...omium.org,
 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 2/3] treewide: Switch memcpy() users of 'task->comm' to
 a more safer implementation

Hi Yafang,

Many thanks for the review.

On 5/22/25 11:57 AM, Yafang Shao wrote:
> On Thu, May 22, 2025 at 2:15 PM Yafang Shao <laoar.shao@...il.com> wrote:
>> On Wed, May 21, 2025 at 2:24 PM Bhupesh <bhupesh@...lia.com> wrote:
>>> As Linus mentioned in [1], currently we have several memcpy() use-cases
>>> which use 'current->comm' to copy the task name over to local copies.
>>> For an example:
>>>
>>>   ...
>>>   char comm[TASK_COMM_LEN];
>>>   memcpy(comm, current->comm, TASK_COMM_LEN);
>>>   ...
>>>
>>> These should be modified so that we can later implement approaches
>>> to handle the task->comm's 16-byte length limitation (TASK_COMM_LEN)
>>> is a more modular way (follow-up patches do the same):
>>>
>>>   ...
>>>   char comm[TASK_COMM_LEN];
>>>   memcpy(comm, current->comm, TASK_COMM_LEN);
>>>   comm[TASK_COMM_LEN - 1] = '\0';
>>>   ...
>>>
>>> The relevant 'memcpy()' users were identified using the following search
>>> pattern:
>>>   $ git grep 'memcpy.*->comm\>'
>> Hello Bhupesh,
>>
>> Several BPF programs currently read task->comm directly, as seen in:
>>
>> // tools/testing/selftests/bpf/progs/test_skb_helpers.c [0]
>> bpf_probe_read_kernel_str(&comm, sizeof(comm), &task->comm);
>>
>> This approach may cause issues after the follow-up patch.
>> I believe we should replace it with the safer bpf_get_current_comm()
>> or explicitly null-terminate it with "comm[sizeof(comm) - 1] = '\0'".
>> Out-of-tree BPF programs like BCC[1] or bpftrace[2] relying on direct
>> task->comm access may also break and require updates.
>>
>> [0]. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/progs/test_skb_helpers.c#n26
>> [1]. https://github.com/iovisor/bcc
>> [2]. https://github.com/bpftrace/bpftrace
> Hmm, upon checking, I confirmed that bpf_probe_read_kernel_str()
> already ensures the destination string is null-terminated. Therefore,
> this change is unnecessary. Please disregard my previous comment.
>

Sure. Yes, bpf_probe_read_kernel_str() handles these cases.

Thanks,
Bhupesh