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

[Yafang Shao <laoar.shao@gmail.com>]

On Sun, Jun 2, 2024 at 11:52 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Yafang Shao <laoar.shao@gmail.com> writes:
>
> > Quoted from Linus [0]:
> >
> >   Since user space can randomly change their names anyway, using locking
> >   was always wrong for readers (for writers it probably does make sense
> >   to have some lock - although practically speaking nobody cares there
> >   either, but at least for a writer some kind of race could have
> >   long-term mixed results
>
> Ugh.
> Ick.
>
> This code is buggy.
>
> I won't argue that Linus is wrong, about removing the
> task_lock.
>
> Unfortunately strscpy_pad does not work properly with the
> task_lock removed, and buf_size larger that TASK_COMM_LEN.
> There is a race that will allow reading past the end
> of tsk->comm, if we read while tsk->common is being
> updated.

It appears so. Thanks for pointing it out. Additionally, other code,
such as the BPF helper bpf_get_current_comm(), also uses strscpy_pad()
directly without the task_lock. It seems we should change that as
well.

>
> So __get_task_comm needs to look something like:
>
> char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> {
>         size_t len = buf_size;
>         if (len > TASK_COMM_LEN)
>                 len = TASK_COMM_LEN;
>         memcpy(buf, tsk->comm, len);
>         buf[len -1] = '\0';
>         return buf;
> }

Thanks for your suggestion.

>
> What shows up in buf past the '\0' is not guaranteed in the above
> version but I would be surprised if anyone cares.

I believe we pad it to prevent the leakage of kernel data. In this
case, since no kernel data will be leaked, the following change may be
unnecessary.

>
> If people do care the code can do something like:
> char *last = strchr(buf);
> memset(last, '\0', buf_size - (last - buf));
>
> To zero everything in the buffer past the first '\0' byte.
>

-- 
Regards
Yafang