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? 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/[email protected]?part=1
