Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] io_uring: UAF of dmabuf map on async I/O reissue with partial progress - [High] io_uring: UAF of driver module due to fput() race in io_release_reg_dmabuf() --
commit 72563100c98fb5b83c9261811926c28b98fe4b8b Author: Pavel Begunkov <[email protected]> io_uring/rsrc: add dmabuf backed registered buffers This commit introduces support for dmabuf-backed registered buffers, allowing users to register a dmabuf file descriptor and a target file. Direct I/O requests can then be issued against the target file using standard io_uring fixed read/write opcodes. > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c > index 6e4fe70b316f0..535cc94a1421f 100644 > --- a/io_uring/rsrc.c > +++ b/io_uring/rsrc.c [ ... ] > +static void io_release_reg_dmabuf(void *priv) > +{ > + struct io_regbuf_dma *db = priv; > + > + fput(db->target_file); > + dma_buf_io_ctx_release(db->ctx); [Severity: High] Could this sequence create a race condition with module unloading? If fput() is called during process exit (PF_EXITING), it might schedule delayed_fput_work on the system workqueue. A concurrent worker thread could execute __fput(), dropping the final module reference of target_file->f_op->owner. If that happens, and the original thread then proceeds to call dma_buf_io_ctx_release(db->ctx) which invokes ctx->dev_ops->release(ctx), would this execute unmapped memory if the target module is unloaded in the interim? > + kfree(db); > +} [ ... ] > diff --git a/io_uring/rw.c b/io_uring/rw.c > index 755166e90746e..9867ae2905e31 100644 > --- a/io_uring/rw.c > +++ b/io_uring/rw.c > @@ -367,8 +367,8 @@ static int io_init_rw_fixed(struct io_kiocb *req, > unsigned int issue_flags, > if (io->bytes_done) > return 0; [Severity: High] Does this early return leave a dangling pointer in the iterator if the request is being reissued after partial progress? If an async I/O request on a dmabuf-backed fixed buffer makes partial progress (io->bytes_done > 0) but then completes asynchronously with -EAGAIN, it appears io_complete_rw() unconditionally calls io_req_drop_dmabuf(), dropping the map reference. When the request is caught and requeued to io-wq (because __io_complete_rw_common() marks it with REQ_F_REISSUE), returning early here bypasses io_import_dmabuf(). Would the iterator then retain a dangling pointer to the previously freed map, leading to a use-after-free when the block layer processes the remaining bytes? > > - ret = io_import_reg_buf(req, &io->iter, rw->addr, rw->len, ddir, > - issue_flags); > + ret = __io_import_reg_buf(req, &io->iter, rw->addr, rw->len, ddir, > + issue_flags, IO_REGBUF_IMPORT_ALLOW_DMABUF); > iov_iter_save_state(&io->iter, &io->iter_state); > return ret; > } [ ... ] > @@ -583,6 +583,8 @@ static void io_complete_rw(struct kiocb *kiocb, long res) > struct io_rw *rw = container_of(kiocb, struct io_rw, kiocb); > struct io_kiocb *req = cmd_to_io_kiocb(rw); > > + io_req_drop_dmabuf(req); [Severity: High] Is it safe to unconditionally drop the dmabuf map here before checking if the request will be reissued? If this request made partial progress but completes with -EAGAIN, __io_complete_rw_common() will see the result and set REQ_F_REISSUE. Since io_req_drop_dmabuf() has already dropped the map, would the reissued request in io-wq end up with a dangling map reference if io_init_rw_fixed() skips re-importing it? > + > /* ring owner may block in freeze_super() before task_work runs */ > if (kiocb->ki_flags & IOCB_WRITE) > io_req_end_write(req); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
