Re: [PATCH v10 07/13] nvme-pci: implement dma-buf backed requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvme: missing controller state check allows attaching DMA-BUF to dead PCI device
--

commit 6741fe43dc1132ae4f51db272567ac5b56987f17
Author: Pavel Begunkov <asml.silence@gmail.com>

nvme-pci: implement dma-buf backed requests

This patch implements DMA-BUF backed requests for the NVMe PCI driver. It
allows mapping DMA-BUFs on registration and storing them as a PRP list to
initialize requests. Attached contexts are tracked in a new list
synchronized with a lock.

> diff --git a/drivers/nvme/host/pci-dmabuf.c b/drivers/nvme/host/pci-dmabuf.c
> new file mode 100644
> index 0000000000000..8dab59d936507
> --- /dev/null
> +++ b/drivers/nvme/host/pci-dmabuf.c

[ ... ]

> +static int nvme_init_io_ctx_locked(struct nvme_ctrl *ctrl,
> +		struct nvme_dma_buf_list *buf_list,
> +		struct dma_buf_io_ctx *ctx)
> +{
> +	struct nvme_dma_buf_io_ctx *nvme_ctx;
> +	struct dma_buf_attachment *attach;
> +
> +	nvme_ctx = kzalloc_obj(*nvme_ctx);
> +	if (!nvme_ctx)
> +		return -ENOMEM;
> +
> +	attach = dma_buf_dynamic_attach(ctx->dmabuf, ctrl->dev,
> +					&nvme_dmabuf_importer_ops, ctx);

[Severity: High]
Does this code allow attaching a DMA-BUF to a tearing-down or dead PCI device?

If the PCI device is hot-unplugged or its driver is unbound, nvme_remove()
calls nvme_dev_disable() which cleans up existing DMA-BUF contexts via
nvme_pci_remove_dmabuf().

Since nvme_get_ctrl() only pins the char device, not the underlying
PCI device binding, could an unprivileged user with an open file descriptor
to the block device invoke the register buffers ioctl and race with
device removal?

> +	if (IS_ERR(attach)) {
> +		kfree(nvme_ctx);
> +		return PTR_ERR(attach);
> +	}
> +
> +	nvme_get_ctrl(ctrl);
> +	list_add(&nvme_ctx->list, &buf_list->ctxs);
> +	nvme_ctx->attach = attach;
> +	nvme_ctx->base = ctx;
> +	nvme_ctx->ctrl = ctrl;
> +	nvme_ctx->buf_list = buf_list;
> +	ctx->dev_priv = nvme_ctx;
> +	ctx->dev_ops = &nvme_dma_buf_io_ops;
> +	return 0;
> +}
> +
> +int __nvme_init_dma_buf_io_ctx(struct nvme_ctrl *ctrl,
> +		struct nvme_dma_buf_list *buf_list,
> +		struct dma_buf_io_ctx *ctx)
> +{
> +	int ret;
> +
> +	mutex_lock(&buf_list->lock);
> +	ret = nvme_init_io_ctx_locked(ctrl, buf_list, ctx);

[Severity: High]
Is a controller state check needed here before proceeding with the attachment?

Without verifying ctrl->state, this function can successfully add a new
attachment to the device's list, mapping it to a PCI device that is no
longer managed by the driver. This could lead to IOMMU domain use-after-free
or memory leaks.

> +	mutex_unlock(&buf_list->lock);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791924581.git.asml.silence@gmail.com?part=7