Re: [PATCHSET v3 0/4] fs: Support for LOOKUP_NONBLOCK / RESOLVE_NONBLOCK (Insufficiently faking current?)
From: Eric W. Biederman <hidden>
Date: 2021-02-15 17:58:02
Jens Axboe [off-list ref] writes:
On 2/13/21 3:08 PM, Eric W. Biederman wrote:quoted
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.
quoted hunk
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 [off-list ref]
Date: Fri Nov 13 16:47:52 2020 -0700
proc: don't allow async path resolution of /proc/self componentsI suspect that would be better as PF_IO_WORKER as well.
quoted
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