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

[Pavel Begunkov <asml.silence@gmail.com>]

On 9/28/26 15:02, sashiko-bot@kernel.org wrote:
...>> +	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

It'd be rejected, not truncated, see below.

> 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?

I can clear it, but unless I missed sth that's what e.g.
nvme_pci_setup_data_prp() already does.

nvme_pci_setup_data_prp() // goto bad_sgl:
	nvme_unmap_data()
		nvme_free_descriptors();


> 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).

It'll fail on the alignment check in this loop for [4096, 4097)

> 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()?

Going to fix.

-- 
Pavel Begunkov