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

[Jens Axboe <axboe@kernel.dk>]

On 2/13/21 3:08 PM, Eric W. Biederman wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> Hi,
>>
>> Wanted to throw out what the current state of this is, as we keep
>> getting closer to something palatable.
>>
>> This time I've included the io_uring change too. I've tested this through
>> both openat2, and through io_uring as well.
>>
>> I'm pretty happy with this at this point. The core change is very simple,
>> and the users end up being trivial too.
>>
>> Also available here:
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=nonblock-path-lookup
>>
>> Changes since v2:
>>
>> - Simplify the LOOKUP_NONBLOCK to _just_ cover LOOKUP_RCU and
>>   lookup_fast(), as per Linus's suggestion. I verified fast path
>>   operations are indeed just that, and it avoids having to mess with
>>   the inode lock and mnt_want_write() completely.
>>
>> - Make O_TMPFILE return -EAGAIN for LOOKUP_NONBLOCK.
>>
>> - Improve/add a few comments.
>>
>> - Folded what was left of the open part of LOOKUP_NONBLOCK into the
>>   main patch.
> 
> 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:

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);
+
 	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

> 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.

-- 
Jens Axboe