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

[sashiko-bot@kernel.org]

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 <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
> 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/cover.1789997898.git.asml.silence@gmail.com?part=6