Re: [PATCH 1/6] fs/exec: Drop task_lock() inside __get_task_comm()

["Eric W. Biederman" <ebiederm@xmission.com>]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Sun, Jun 2, 2024 at 10:53 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> If you are performing lockless reads and depending upon a '\0'
>> terminator without limiting yourself to the size of the buffer
>> there needs to be a big fat comment as to how in the world
>> you are guaranteed that a '\0' inside the buffer will always
>> be found.
>
> I think Yafang can certainly add such a comment next to
> __[gs]et_task_comm.
>
> I prefer to avoid open coding memcpy + mmemset when strscpy_pad works.

Looking through the code in set_task_comm
strscpy_pad only works when both the source and designation are aligned.
Otherwise it performs a byte a time copy, and is most definitely
susceptible to the race I observed.

Further I looked a couple of the uses of set_task_com, in
fs/proc/base.c, kernel/kthread.c, and kernel/sys.c.

Nowhere do I see a guarantee that the source buffer is word aligned
or even something that would reasonably cause a compiler to place the
buffer that is being passed to set_task_comm to be word aligned.

As far as I can tell it is completely up to the compiler if it will
cause strscpy_pad to honor the word at a time guarantee needed
to make strscpy_pad safe for reading the information.

This is not to say we can't make it safe.

The easiest would be to create an aligned temporary buffer in
set_task_comm, and preserve the existing interface.  Alternatively
a type that has the appropriate size and alignment could be used
as input to set_task_comm and it could be caller's responsibility
to use it.

While we can definitely make reading task->comm happen without taking
the lock.  Doing so without updating set_task_comm to provide the
guarantees needed to make it safe, looks like a case of play silly
games, win silly prizes.

Eric