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 <[email protected]> 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/[email protected]?part=6
