Re: [PATCHSET v3 0/4] fs: Support for LOOKUP_NONBLOCK / RESOLVE_NONBLOCK (Insufficiently faking current?)

[ebiederm@xmission.com (Eric W. Biederman)]

Jens Axboe <axboe@kernel.dk> writes:

> On 2/13/21 3:08 PM, Eric W. Biederman wrote:
>> 
>> I have to ask.  Are you doing work to verify that performing
>> path traversals and opening of files yields the same results
>> when passed to io_uring as it does when performed by the caller?
>> 
>> Looking at v5.11-rc7 it appears I can arbitrarily access the
>> io-wq thread in proc by traversing "/proc/thread-self/".
>
> Yes that looks like a bug, it needs similar treatment to /proc/self:
Agreed.


> diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
> index a553273fbd41..e8ca19371a36 100644
> --- a/fs/proc/thread_self.c
> +++ b/fs/proc/thread_self.c
> @@ -17,6 +17,13 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
>  	pid_t pid = task_pid_nr_ns(current, ns);
>  	char *name;
>  
> +	/*
> +	 * Not currently supported. Once we can inherit all of struct pid,
> +	 * we can allow this.
> +	 */
> +	if (current->flags & PF_KTHREAD)
> +		return ERR_PTR(-EOPNOTSUPP);

I suspect simply testing for PF_IO_WORKER is a better test.  As it is
the delegation to the io_worker not the fact that it is a kernel thread
that causes a problem.

I have a memory of that point being made when the smack_privileged test
and the tomoyo_kernel_service test and how to fix them was being
discussed.

>  	if (!pid)
>  		return ERR_PTR(-ENOENT);
>  	name = kmalloc(10 + 6 + 10 + 1, dentry ? GFP_KERNEL : GFP_ATOMIC);
>
> as was done in this commit:
>
> commit 8d4c3e76e3be11a64df95ddee52e99092d42fc19
> Author: Jens Axboe <axboe@kernel.dk>
> Date:   Fri Nov 13 16:47:52 2020 -0700
>
>     proc: don't allow async path resolution of /proc/self components

I suspect that would be better as PF_IO_WORKER as well.

>> Similarly it looks like opening of "/dev/tty" fails to
>> return the tty of the caller but instead fails because
>> io-wq threads don't have a tty.
>> 
>> 
>> There are a non-trivial number of places where current is used in the
>> kernel both in path traversal and in opening files, and I may be blind
>> but I have not see any work to find all of those places and make certain
>> they are safe when called from io_uring.  That worries me as that using
>> a kernel thread instead of a user thread could potential lead to
>> privilege escalation.
>
> I've got a patch queued up for 5.12 that clears ->fs and ->files for the
> thread if not explicitly inherited, and I'm working on similarly
> proactively catching these cases that could potentially be
> problematic.

Any pointers or is this all in a private tree for now?

It is difficult to follow because many of the fixes have not CC'd the
reporters or even the maintainers of the subsystems who have been
affected by this kind of issue.

Eric