Re: [PATCH v7 13/13] io_uring/rsrc: add dmabuf backed registered buffers

[sashiko-bot@kernel.org]

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 <asml.silence@gmail.com>

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/cover.1790602223.git.asml.silence@gmail.com?part=13