On 9/28/26 15:02, [email protected] 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
