Re: io_uring: risky use of task work, especially wrt fdget()

[Jens Axboe <axboe@kernel.dk>]

On 11/27/23 2:53 PM, Jann Horn wrote:
> Hi!
> 
> I noticed something that I think does not currently cause any
> significant security issues, but could be problematic in the future:
> 
> io_uring sometimes processes task work in the middle of syscalls,
> including between fdget() and fdput(). My understanding of task work
> is that it is expected to run in a context similar to directly at
> syscall entry/exit: task context, no locks held, sleeping is okay, and
> it doesn't execute in the middle of some syscall that expects private
> state of the task_struct to stay the same.
> 
> An example of another user of task work is the keyring subsystem,
> which does task_work_add() in keyctl_session_to_parent() to change the
> cred pointers of another task.
> 
> Several places in io_uring process task work while holding an fdget()
> reference to some file descriptor. For example, the io_uring_enter
> syscall handler calls io_iopoll_check() while the io_ring_ctx is only
> referenced via fdget(). This means that if there were another kernel
> subsystem that uses task work to close file descriptors, io_uring
> would become unsafe. And io_uring does _almost_ that itself, I think:
> io_queue_worker_create() can be run on a workqueue, and uses task work
> to launch a worker thread from the context of a userspace thread; and
> this worker thread can then accept commands to close file descriptors.
> Except it doesn't accept commands to close io_uring file descriptors.
> 
> A closer miss might be io_sync_cancel(), which holds a reference to
> some normal file with fdget()/fdput() while calling into
> io_run_task_work_sig(). However, from what I can tell, the only things
> that are actually done with this file pointer are pointer comparisons,
> so this also shouldn't have significant security impact.
> 
> Would it make sense to use fget()/fput() instead of fdget()/fdput() in
> io_sync_cancel(), io_uring_enter and io_uring_register? These
> functions probably usually run in multithreaded environments anyway
> (thanks to the io_uring worker threads), so I would think fdget()
> shouldn't bring significant performance savings here?

Let me run some testing on that. It's a mistake to think that it's
usually multithreaded, generally if you end up using io-wq then it's not
a fast path. A fast networked setup, for example, would never touch the
threads and hence no threading would be implied by using io_uring. Ditto
on the storage front, if you're just reading/writing or eg doing polled
IO. That said, those workloads are generally threaded _anyway_ - not
because of io_uring, but because that's how these kinds of workloads are
written to begin with.

So probably won't be much of a concern to do the swap. The only
"interesting" part of the above mix of cancel/register/enter is
obviously the enter part. The rest are not really fast path.

-- 
Jens Axboe