Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] nvme: deadlock in nvme_dev_disable() during device removal - [High] nvme: illegal use of single-mapping sync API on sg-mapped dmabuf - [High] nvme: heap buffer overflow in nvme_dma_buf_io_map() - [High] nvme: memory leak of dmabuf map in unmap path - [High] nvme: controller reset permanently breaks dmabuf registrations --
commit 1bee4d32dec1291a7dc832efe4620264ba2425af 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 > index a953c0697f994..5a45710a1daf2 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c [ ... ] > @@ -865,6 +876,140 @@ static void nvme_free_descriptors(struct request *req) > +static void nvme_dmabuf_map_sync_for_cpu(struct nvme_dev *nvme_dev, > + struct request *req) > +{ [ ... ] > + while (length > 0) { > + dma_sync_single_for_cpu(dev, dma_list[map_idx++], > + NVME_CTRL_PAGE_SIZE, dma_dir); > + length -= NVME_CTRL_PAGE_SIZE; > + } > +} [Severity: High] Does this code unlawfully use the single-mapping sync API on an sg-mapped address? The dmabuf memory is mapped using dma_buf_map_attachment() which uses dma_map_sg(). The driver then illegally syncs sub-pages of this scatterlist using dma_sync_single_for_cpu() in nvme_dmabuf_map_sync_for_cpu() and dma_sync_single_for_device() in nvme_dmabuf_map_sync_for_device(). This violates the DMA API. On platforms with SWIOTLB, this can crash or cause silent data corruption because the bounce buffer metadata expects the exact original dma_addr_t and size mapped by the sg API, not piecemeal chunks. > + > +static void nvme_dmabuf_map_sync_for_device(struct nvme_dev *nvme_dev, > + struct request *req) > +{ [ ... ] > + while (length > 0) { > + dma_sync_single_for_device(dev, dma_list[map_idx++], > + NVME_CTRL_PAGE_SIZE, dma_dir); > + length -= NVME_CTRL_PAGE_SIZE; > + } > +} [ ... ] > +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 = kmalloc_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) { > + ret = -EINVAL; > + goto err; > + } > + while (sg_len) { > + map->dma_list[i++] = dma_addr; > + dma_addr += NVME_CTRL_PAGE_SIZE; > + sg_len -= NVME_CTRL_PAGE_SIZE; > + } [Severity: High] Can an attacker controlling the dmabuf size trigger a heap buffer overflow here? In nvme_dma_buf_io_map(), nr_entries is calculated as ctx->dmabuf->size / NVME_CTRL_PAGE_SIZE, truncating down if the size is unaligned. The flex array map is allocated for nr_entries. The sg_table returned by the exporter covers the rounded-up size. The loop iterates based on sg_len and writes unconditionally to map->dma_list. If size was unaligned, sg_len will exceed the truncated calculated size, causing the loop to write past the end of the kmalloc_flex allocation. > + } > + > + 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); > + return ERR_PTR(ret); > +} > + > +static void nvme_dma_buf_io_unmap(struct dma_buf_io_ctx *ctx, > + struct dma_buf_io_map *map_base) > +{ > + struct nvme_dma_buf_io_ctx *nvme_ctx = ctx->dev_priv; > + struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(map_base); > + > + dma_resv_assert_held(ctx->dmabuf->resv); > + > + dma_buf_unmap_attachment(nvme_ctx->attach, map->sgt, ctx->dir); > +} [Severity: High] Does this code leak the memory of the map structure and percpu data on every unmap operation? nvme_dma_buf_io_map() allocates the map structure using kmalloc_flex(). The framework calls dev_ops->unmap (nvme_dma_buf_io_unmap) when the map is released. nvme_dma_buf_io_unmap() unmaps the attachment but fails to call kfree(map) and percpu_ref_exit(&map->refs). [ ... ] > +static void nvme_pci_remove_dmabuf(struct nvme_dev *dev) > +{ > + struct nvme_dma_buf_io_ctx *ctx, *tmp; > + > + mutex_lock(&dev->dmabuf_lock); > + list_for_each_entry_safe(ctx, tmp, &dev->dmabuf_ctxs, list) { > + dma_buf_io_detach(ctx->ctx); > + dma_buf_detach(ctx->ctx->dmabuf, ctx->attach); > + list_del_init(&ctx->list); > + } > + mutex_unlock(&dev->dmabuf_lock); > +} [Severity: High] Does this permanently break dmabuf registrations across transient transport resets? During a transient controller reset (e.g. from an IO timeout), nvme_dev_disable() calls nvme_pci_remove_dmabuf(). This permanently detaches the dmabuf and marks the context's maps as killed. The NVMe controller resets and comes back online, but the userspace registered buffers are permanently destroyed and will return -ENODEV. This breaks the intended transparency of NVMe resets, causing IO submissions using registered dmabufs to fail unrecoverably until userspace re-registers them. [ ... ] > @@ -3316,6 +3633,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool > shutdown) > struct pci_dev *pdev = to_pci_dev(dev->dev); > bool dead; > > + nvme_pci_remove_dmabuf(dev); > + > mutex_lock(&dev->shutdown_lock); [Severity: Critical] Could this sequence result in an ABBA deadlock preventing device recovery? nvme_dev_disable() calls nvme_pci_remove_dmabuf(dev) before calling nvme_cancel_tagset(). nvme_pci_remove_dmabuf() calls dma_buf_io_detach() which blocks waiting for all active dmabuf IOs to complete. Since the device is dead or resetting, the hardware won't complete the IOs. The IOs must be aborted by nvme_cancel_tagset(), but the thread is permanently blocked waiting for them. This was not resolved in subsequent patches in the series. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
