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

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

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.

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;
}

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

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.


Eric


> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
>  fs/exec.c             | 7 +++++--
>  include/linux/sched.h | 2 +-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index b3c40fbb325f..b43992d35a8a 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1227,12 +1227,15 @@ static int unshare_sighand(struct task_struct *me)
>  	return 0;
>  }
>  
> +/*
> + * User space can randomly change their names anyway, so locking for readers
> + * doesn't make sense. For writers, locking is probably necessary, as a race
> + * condition could lead to long-term mixed results.
> + */
>  char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
>  {
> -	task_lock(tsk);
>  	/* Always NUL terminated and zero-padded */
>  	strscpy_pad(buf, tsk->comm, buf_size);
> -	task_unlock(tsk);
>  	return buf;
>  }
>  EXPORT_SYMBOL_GPL(__get_task_comm);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c75fd46506df..56a927393a38 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1083,7 +1083,7 @@ struct task_struct {
>  	 *
>  	 * - normally initialized setup_new_exec()
>  	 * - access it with [gs]et_task_comm()
> -	 * - lock it with task_lock()
> +	 * - lock it with task_lock() for writing
>  	 */
>  	char				comm[TASK_COMM_LEN];