Re: [PATCH v7 06/13] nvme-pci: implement dma-buf backed requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] block: out-of-bounds heap read in nvme_rq_setup_dmabuf_map allows arbitrary DMA
- [High] mm: invalid kfree() on kvmalloc_flex() allocation in nvme_dma_buf_io_map
- [High] mm: heap buffer overflow in nvme_dma_buf_io_map due to truncated nr_entries
- [High] mm: double free of PRP lists on nvme_rq_setup_dmabuf_map error path
--

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

nvme-pci: implement dma-buf backed requests

Enable BIO_DMABUF_MAP backed requests. On registration we map the
dma-buf and store it as a prp list, which is then used to initialise
requests. All attached contexts are stored in a new list dmabuf_ctxs,
and additions/removals are synchronised with dmabuf_lock.

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c

[ ... ]

> +static void nvme_rq_clean_dmabuf_map(struct nvme_dev *dev,
> +		struct request *req)
> +{
> +	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> +
> +	nvme_dmabuf_map_sync_for_cpu(dev, req);
> +
> +	if (iod->nr_descriptors)
> +		nvme_free_descriptors(req);
> +}

[ ... ]

> +static blk_status_t nvme_rq_setup_dmabuf_map(struct request *req,
> +		struct nvme_queue *nvmeq)
> +{
> +	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> +	struct bio *bio = req->bio;
> +	struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
> +	unsigned bvec_done = bio->bi_iter.bi_offset;
> +	unsigned map_idx = bvec_done / NVME_CTRL_PAGE_SIZE;
> +	unsigned offset = bvec_done & (NVME_CTRL_PAGE_SIZE - 1);
> +	int length = blk_rq_payload_bytes(req) - (NVME_CTRL_PAGE_SIZE - offset);
> +	dma_addr_t *dma_list = map->dma_list;
> +	u64 prp1_dma = dma_list[map_idx++] + offset;
> +	u64 dma_addr, prp2_dma;
> +	dma_addr_t prp_dma;
> +	__le64 *prp_list;
> +	unsigned i;
> +
> +	nvme_dmabuf_map_sync_for_device(nvmeq->dev, req);
> +
> +	if (length <= 0) {
> +		prp2_dma = 0;
> +		goto done;
> +	}
> +
> +	if (length <= NVME_CTRL_PAGE_SIZE) {
> +		prp2_dma = dma_list[map_idx];

[Severity: Critical]
Is map_idx checked against map->nr_entries here?

If dmabuf->size is 4608 bytes, nr_entries is truncated to 1, allocating 1
array element. Userspace could submit a 4608-byte IO. The driver calculates
'length' up to 4608 and advances 'map_idx' to 1 for the remaining 512 bytes.
It reads 'dma_list[1]' (which is out-of-bounds) and uses it as 'prp2_dma',
potentially resulting in arbitrary physical DMA (read or write).

> +		goto done;
> +	}

[ ... ]

> +free_prps:
> +	iod->cmd.common.dptr.prp1 = cpu_to_le64(prp1_dma);
> +	iod->cmd.common.dptr.prp2 = cpu_to_le64(prp2_dma);
> +	nvme_free_descriptors(req);
> +	return BLK_STS_RESOURCE;

[Severity: High]
Does this code leave iod->nr_descriptors greater than 0 on the error path?

If dma_pool_alloc() fails in nvme_rq_setup_dmabuf_map(), it jumps to
'free_prps:', which calls nvme_free_descriptors() but leaves
'iod->nr_descriptors' unchanged. The block layer may requeue the failed
request.

If the queue is subsequently torn down, the cancellation path invokes
nvme_unmap_data() -> nvme_rq_clean_dmabuf_map(). Seeing the non-zero
'nr_descriptors', it calls nvme_free_descriptors() again, resulting in a
double free.

> +}

[ ... ]

> +static struct dma_buf_io_map *nvme_dma_buf_io_map(struct dma_buf_io_ctx *ctx)
> +{
> +	unsigned nr_entries = ctx->dmabuf->size / NVME_CTRL_PAGE_SIZE;
> +	struct nvme_dma_buf_io_ctx *nvme_ctx = ctx->dev_priv;
> +	struct dma_buf_attachment *attach = nvme_ctx->attach;
> +	unsigned long tmp, i = 0;
> +	struct nvme_dmabuf_map *map;
> +	struct scatterlist *sg;
> +	struct sg_table *sgt;
> +	int ret;
> +
> +	dma_resv_assert_held(ctx->dmabuf->resv);
> +
> +	map = kvmalloc_flex(*map, dma_list, nr_entries);
> +	if (!map)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sgt = dma_buf_map_attachment(attach, ctx->dir);
> +	if (IS_ERR(sgt)) {
> +		ret = PTR_ERR(sgt);
> +		sgt = NULL;
> +		goto err;
> +	}
> +
> +	for_each_sgtable_dma_sg(sgt, sg, tmp) {
> +		dma_addr_t dma_addr = sg_dma_address(sg);
> +		unsigned long sg_len = sg_dma_len(sg);
> +
> +		if ((sg_len % NVME_CTRL_PAGE_SIZE) ||
> +		    (dma_addr % NVME_CTRL_PAGE_SIZE)) {
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +		while (sg_len) {
> +			map->dma_list[i++] = dma_addr;

[Severity: High]
Does this loop overflow dma_list[]?

'nr_entries' is computed via integer division of dmabuf->size. If the dmabuf
size is not page-aligned (e.g., 4097 bytes), nr_entries truncates down (to 1).
However, exporters often pad sg_table allocations to full pages (e.g., 8192
bytes).

The loop unconditionally writes all sg_len chunks to 'dma_list', exceeding the
allocated 'nr_entries' capacity.

> +			dma_addr += NVME_CTRL_PAGE_SIZE;
> +			sg_len -= NVME_CTRL_PAGE_SIZE;
> +		}
> +	}
> +
> +	ret = dma_buf_io_init_map(ctx, &map->base, sgt);
> +	if (ret)
> +		goto err;
> +	map->nr_entries = nr_entries;
> +	map->sgt = sgt;
> +	return &map->base;
> +err:
> +	if (sgt)
> +		dma_buf_unmap_attachment(attach, sgt, ctx->dir);
> +	kfree(map);

[Severity: High]
Is it safe to free a kvmalloc_flex() allocation with kfree()?

If the dmabuf setup fails, the code jumps to the 'err' label and executes
kfree(map). Calling kfree() on memory allocated via vmalloc() causes undefined
behavior, typically a kernel panic.

> +	return ERR_PTR(ret);
> +}

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