Re: [PATCH v7 1/9] vfio/pci: Remove DMABUF export dependency on vdev->memory_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vfio: incorrect initial DMABUF revoked state allows DMA to disabled BARs
--

commit fb51fe9e3cab37471d1c4237de83fcf89aad79f5
Author: Matt Evans <matt@ozlabs.org>

vfio/pci: Remove DMABUF export dependency on vdev->memory_lock

This commit introduces a new dmabuf_lock to protect the list of exported
DMABUFs and a new bars_revoked flag for tracking BAR revocation state. This
removes the dependency on vdev->memory_lock during DMABUF export in
preparation for future mmap refactoring.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 6757054e9d875..1f60661078313 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -663,6 +663,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  		vdev->has_vga = true;
>  
>  	vfio_pci_core_map_bars(vdev);
> +	vdev->bars_revoked = false;

[Severity: High]
Is this hardcoded initialization safe?

Earlier in vfio_pci_core_enable(), pci_try_reset_function() is called,
which performs a fundamental reset that clears the device's
PCI_COMMAND_MEMORY bit in the physical hardware.

Then vfio_config_init() populates the virtual config space by reading from
the hardware, capturing PCI_COMMAND_MEMORY == 0. Does initializing
bars_revoked to false here fail to match that initial virtual config state?

>  
>  	return 0;
>  

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index c16f460c01d68..847ee5cc78e7f 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c

[ ... ]

> @@ -305,12 +305,27 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
>  
>  	/* dma_buf_put() now frees priv */
>  	INIT_LIST_HEAD(&priv->dmabufs_elm);
> -	down_write(&vdev->memory_lock);
> +
> +	/*
> +	 * dmabuf_lock synchronises access (R) or updates (W) to the
> +	 * vdev->dmabufs list and to bars_revoked (see below).  The
> +	 * revocation state of DMABUF elements in the list is written
> +	 * holding both dmabuf_lock(W) and resv, and tested with
> +	 * either.
> +	 *
> +	 * dmabuf_lock -> resv
> +	 *
> +	 * vdev->bars_revoked tracks the BAR revocation status updated
> +	 * via vfio_pci_dma_buf_move(), so the initial DMABUF state
> +	 * follows the same criteria that later update the DMABUF
> +	 * state (BAR zap, etc.).
> +	 */
> +	down_write(&vdev->dmabuf_lock);
>  	dma_resv_lock(priv->dmabuf->resv, NULL);
> -	priv->revoked = !__vfio_pci_memory_enabled(vdev);
> +	priv->revoked = vdev->bars_revoked;

[Severity: High]
Can this incorrect initial state allow P2P DMA to disabled BARs?

If userspace exports a DMABUF at this point, the DMABUF inherits the
un-revoked state from vdev->bars_revoked (false), bypassing the correct
state evaluation.

Could this permit P2P DMA to this DMABUF to be allowed by the kernel but
dropped by the endpoint since memory decoding is disabled? This would trigger
PCIe Unsupported Request (UR) errors, which could potentially be weaponized
by unprivileged users controlling the VFIO device to trigger AER and crash
the host.

>  	list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs);
>  	dma_resv_unlock(priv->dmabuf->resv);
> -	up_write(&vdev->memory_lock);
> +	up_write(&vdev->dmabuf_lock);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924152159.49702-1-matt@ozlabs.org?part=1