Re: [PATCH 4/5] io_uring/dma-buf: switch to tag file filtering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] io_uring/dma-buf: unpinned gendisk tag after fput() in __io_register_dmabuf()
- [Critical] io_uring/dma-buf: uninitialized f_dma_io_tag in init_file() allows stale match
- [High] block: use-after-free of bdev in blkdev_open() after blkdev_put_no_open()
--

commit f539898d6b6343a21d00edd1090b62db7611212c
Author: Pavel Begunkov <asml.silence@gmail.com>

io_uring/dma-buf: switch to tag file filtering

This commit switches io_uring registered dma-buf buffers from matching the
exact registered target_file pointer to checking file compatibility via
dma_buf_io_file_compatible(), and drops target_file immediately upon
registration.

> diff --git a/io_uring/dma-buf.c b/io_uring/dma-buf.c
> index 6f1d21c0f558f..f25749f7da22b 100644
> --- a/io_uring/dma-buf.c
> +++ b/io_uring/dma-buf.c
> @@ -4,7 +4,6 @@ static inline void io_release_dmabuf(void *priv)
>  {
>  	struct io_buf_dma *bd = io_priv_to_dmabuf(priv);
>  
> -	fput(bd->target_file);
>  	dma_buf_io_ctx_release(bd->ctx);
>  	kfree(bd);
>  }
> @@ -52,13 +51,13 @@ int __io_register_dmabuf(struct io_ring_ctx *ctx,
>  	if (ret)
>  		goto err;
>  
> -	bd->target_file = target_file;
>  	imu->len = dmabuf->size;
>  	imu->release = io_release_dmabuf;
>  	imu->priv = bd;
>  	imu->flags = IO_REGBUF_F_DMABUF;
>  	imu->dir = IO_BUF_DEST | IO_BUF_SOURCE;
>  	dma_buf_put(dmabuf);
> +	fput(target_file);

[Severity: Critical]
Does dropping target_file in __io_register_dmabuf() instead of holding it
until io_release_dmabuf() leave the struct gendisk pointer stored in
ctx->importer_tag unpinned, allowing ABA tag aliasing if the disk is freed?

When a block device is opened, blkdev_open() sets filp->f_dma_io_tag to the
raw struct gendisk pointer:

block/fops.c:blkdev_open() {
	...
	filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;
}

During registration, dma_buf_io_ctx_create() saves target_file->f_dma_io_tag
into ctx->importer_tag, while nvme_init_dma_buf_io_ctx() ->
nvme_init_io_ctx_locked() only calls nvme_get_ctrl(ctrl) without taking a
reference on target_file, bdev, struct gendisk, or struct nvme_ns:

drivers/nvme/host/pci-dmabuf.c:nvme_init_io_ctx_locked() {
	...
	nvme_get_ctrl(ctrl);
	list_add(&nvme_ctx->list, &buf_list->ctxs);
}

Previously, keeping bd->target_file until io_release_dmabuf() pinned bdev and
bdev->bd_disk for the lifetime of the registered buffer. With fput(target_file)
called immediately in __io_register_dmabuf(), closing target_fd and removing
the namespace (via nvme_ns_remove() -> del_gendisk() -> nvme_put_ns() ->
nvme_free_ns() -> put_disk(ns->disk)) can free the struct gendisk while
bd->ctx remains active.

If a new struct gendisk is later allocated at the same address for a
different block device (such as another NVMe controller/IOMMU domain or a
non-NVMe driver like loop, scsi, virtio_blk, or dm), opening that device will
assign the same f_dma_io_tag address. Subsequent IORING_OP_READ_FIXED or
IORING_OP_WRITE_FIXED requests on the new file will then pass
dma_buf_io_file_compatible() in io_import_dmabuf_map() and submit REQ_DMABUF
bios (where bio->bi_dmabuf_map aliases bio->bi_io_vec with bi_max_vecs == 0)
to an incompatible device or driver.

> diff --git a/io_uring/dma-buf.h b/io_uring/dma-buf.h
> index a97412ebbb3fb..9bd7c8c643976 100644
> --- a/io_uring/dma-buf.h
> +++ b/io_uring/dma-buf.h

[ ... ]

> @@ -63,11 +62,10 @@ static inline int io_import_dmabuf_map(struct io_kiocb *req,
>  				struct io_mapped_ubuf *imu,
>  				size_t len, size_t offset)
>  {
> -	struct io_buf_dma *bd = io_buf_to_dmabuf(imu);
> -
>  	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
>  		return -EOPNOTSUPP;
> -	if (req->file != bd->target_file)
> +	if (unlikely(!dma_buf_io_file_compatible(io_buf_to_dmabuf(imu)->ctx,
> +						 req->file)))
>  		return -EBADF;

[Severity: Critical]
Can dma_buf_io_file_compatible() in io_import_dmabuf_map() match unrelated
files due to uninitialized f_dma_io_tag in init_file()?

In fs/file_table.c, alloc_empty_file() and alloc_empty_file_noaccount()
allocate struct file objects from filp_cache (which uses SLAB_TYPESAFE_BY_RCU
without __GFP_ZERO or a slab constructor), and init_file() initializes fields
individually without clearing f->f_dma_io_tag:

fs/file_table.c:init_file() {
	...
	f->f_iocb_flags = 0;
	f->f_pos	= 0;
	f->f_wb_err	= 0;
	f->f_sb_err	= 0;

	file_ref_init(&f->f_ref, 1);
	return 0;
}

When a block device file is closed (including target_file after
__io_register_dmabuf() calls fput(target_file)), its struct file is returned
to filp_cache still holding its stale f_dma_io_tag value. If that struct file
is recycled for a non-block file (for example, an O_DIRECT regular file on a
filesystem backed by loop, scsi, virtio_blk, dm, or another NVMe controller),
req->file->f_dma_io_tag will still equal ctx->importer_tag.

Wouldn't dma_buf_io_file_compatible() then return true in
io_import_dmabuf_map(), allowing ITER_DMABUF_MAP / REQ_DMABUF bios (with
bio->bi_io_vec aliased to struct dma_buf_io_map *) to be submitted to
incompatible filesystems and block drivers?

[Severity: High]
Also related to how f_dma_io_tag is populated for dma_buf_io_file_compatible()
in this series, in block/fops.c:blkdev_open():

block/fops.c:blkdev_open() {
	...
	ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
	if (ret)
		blkdev_put_no_open(bdev);
	filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;
	return ret;
}

When bdev_open() fails, blkdev_put_no_open(bdev) drops the reference on bdev
acquired by blkdev_get_no_open(). Doesn't setting filp->f_dma_io_tag =
(unsigned long)bdev->bd_disk right after blkdev_put_no_open(bdev) cause a
use-after-free read of bdev?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793751001.git.asml.silence@gmail.com?part=4