Re: [PATCH 16/26] RDMA/uverbs: Use the SGT_P2P mapping type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/uverbs: use-after-free in uverbs_dmabuf_match_mapping() after unbind
--

commit 573a52ef0d548646c203780dd26bd3aa8dbc3284
Author: Jason Gunthorpe <jgg@ziepe.ca>

RDMA/uverbs: Use the SGT_P2P mapping type

This commit converts the RDMA uverbs dma-buf exporter to use the SGT_P2P
mapping type via uverbs_dmabuf_match_mapping() and removes the
uverbs_dmabuf_attach() callback.

> diff --git a/drivers/infiniband/core/uverbs_std_types_dmabuf.c b/drivers/infiniband/core/uverbs_std_types_dmabuf.c
> index 2411ebee69e2d..3c5c3ea2cf556 100644
> --- a/drivers/infiniband/core/uverbs_std_types_dmabuf.c
> +++ b/drivers/infiniband/core/uverbs_std_types_dmabuf.c

[ ... ]

> @@ -76,13 +67,30 @@ static void uverbs_dmabuf_release(struct dma_buf *dmabuf)
>  	uverbs_uobject_release(&priv->uobj);
>  }
>  
> -static const struct dma_buf_ops uverbs_dmabuf_ops = {
> -	.attach = uverbs_dmabuf_attach,
> +static const struct dma_buf_mapping_sgt_exp_ops uverbs_dma_buf_sgt_ops = {
>  	.map_dma_buf = uverbs_dmabuf_map,
>  	.unmap_dma_buf = uverbs_dmabuf_unmap,
> +};
> +
> +static int uverbs_dmabuf_match_mapping(struct dma_buf_match_args *args)
> +{
> +	struct ib_uverbs_dmabuf_file *priv = args->dmabuf->priv;
> +	struct dma_buf_mapping_match sgt_match[1];
> +
> +	if (WARN_ON(!dev_is_pci(priv->provider->owner)))
> +		return -EINVAL;
> +
> +	sgt_match[0] = DMA_BUF_EMAPPING_SGT_P2P(
> +		&uverbs_dma_buf_sgt_ops, to_pci_dev(priv->provider->owner));
> +
> +	return dma_buf_match_mapping(args, sgt_match, ARRAY_SIZE(sgt_match));
> +}

[Severity: High]
Can uverbs_dmabuf_match_mapping() cause a use-after-free of priv->provider
and priv->provider->owner if an importer attaches to the dma-buf after the
exporting PCI/RDMA driver unbinds or the PCI device is removed?

During UVERBS_METHOD_DMABUF_ALLOC, priv->provider is populated via
ib_dev->ops.mmap_get_pfns() -> mlx5_ib_mmap_get_pfns() ->
pcim_p2pdma_provider():

drivers/pci/p2pdma.c:pcim_p2pdma_provider() {
	...
	p2p = rcu_dereference_protected(pdev->p2pdma, 1);
	if (WARN_ON(!p2p))
		/* Someone forgot to call to pcim_p2pdma_init() before */
		return NULL;

	return &p2p->mem[bar];
}

Because struct pci_p2pdma is allocated with devm_kzalloc(&pdev->dev, ...) in
pcim_p2pdma_init(), its lifetime is tied to the PCI driver binding rather
than the exported dma-buf file.

When the driver is unbound or hot-unplugged while userspace still holds the
dma-buf fd open, ib_uverbs_remove_one() calls ib_uverbs_free_hw_resources()
-> uverbs_destroy_ufile_hw() -> uverbs_dmabuf_fd_destroy_uobj():

drivers/infiniband/core/uverbs_std_types_dmabuf.c:uverbs_dmabuf_fd_destroy_uobj() {
	...
	dma_resv_lock(uverbs_dmabuf->dmabuf->resv, NULL);
	if (!uverbs_dmabuf->revoked) {
		uverbs_dmabuf->revoked = true;
		list_del(&uverbs_dmabuf->dmabufs_elm);
		dma_buf_invalidate_mappings(uverbs_dmabuf->dmabuf);
		dma_resv_wait_timeout(uverbs_dmabuf->dmabuf->resv,
				      DMA_RESV_USAGE_BOOKKEEP, false,
				      MAX_SCHEDULE_TIMEOUT);
		wait_for_comp = true;
	}
	dma_resv_unlock(uverbs_dmabuf->dmabuf->resv);
	...
}

After revoking and waiting for active mappings to unmap,
ib_uverbs_remove_one() sets wait_clients = 0 and returns without waiting for
open dma-buf fds to close. Once the PCI driver remove completes, devres frees
struct pci_p2pdma (leaving priv->provider dangling), and if the PCI device is
removed, struct pci_dev (priv->provider->owner) can also be freed since
struct ib_uverbs_dmabuf_file does not hold a pci_dev_get() reference.

Previously, uverbs_dmabuf_attach() only checked attachment->peer2peer, and
priv->provider was only accessed inside uverbs_dmabuf_map() under
dma_resv_lock() after checking !priv->revoked.

With this change, dma_buf_mapping_attach() invokes
uverbs_dmabuf_match_mapping() without holding dmabuf->resv or checking
priv->revoked, and unconditionally dereferences priv->provider->owner and
passes the pci_dev to dma_buf_match_mapping() -> dma_buf_sgt_match() ->
pci_p2pdma_distance().

Notice that commit 9770a3eb805c ("vfio/pci: Change the DMA-buf exporter to
use mapping_type") earlier in this series avoided this same issue in
vfio_pci_dmabuf.c by storing a reference-counted struct pci_dev pointer in
priv->pdev for the lifetime of the dma-buf and passing priv->pdev in
match_mapping(). Should uverbs_std_types_dmabuf.c similarly hold a reference
or synchronize against revocation here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026053623.202642-1-apopple@nvidia.com?part=16