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
