Dear Robot,
On 24/09/2026 16:37, [email protected] wrote:
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 <[email protected]>
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?
I believe so -- this report seems to be incorrect.
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.
First, on entering any driver probe, the PCIe function is enabled (MSE=1).
The HW FLR does indeed clear MSE, but pci_try_reset_function()
saves/restores config space either side of the reset: since it starts
out enabled, the pci_dev_restore(dev) in there means that
PCI_COMMAND_MEMORY is re-enabled after the reset.
And for a device that doesn't have a reset method, MSE is never cleared
so is also enabled through vfio_pci_core_enable().
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?
vfio_config_init() runs after the above has restored/left MSE=1, so the
vconfig captures memory enabled.
So, vdev->bars_revoked = false is correct here. (I had considered
initialising it based on the read value of MSE, but I can't see a way
for MSE to be 0 here.)
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?
Nope.
If userspace exports a DMABUF at this point,
Above, the flag value is correct, but also one can't export a DMABUF
from a VFIO device that hasn't completed initialisation.
Matt
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);
