Re: [PATCH v2 11/12] virtio_pci: support VIRTIO_F_DMB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds MMIO read in `vp_dmb_mem_type_supported` due to `mdev->common_len` exceeding the actual mapped BAR size.
--

--- Patch [11]: [PATCH v2 11/12] virtio_pci: support VIRTIO_F_DMB ---
commit 2b2d786abf770566580dde39b790350db36f5d09
Author: Alexander Graf <graf@amazon.com>

virtio_pci: support VIRTIO_F_DMB

Let a modern virtio-pci device place its virtqueues and the buffers they
reference in a Device Memory Buffer of its own: accept VIRTIO_F_DMB from
vp_transport_features(), and implement the get_dmb_shm_id config op on
top of vp_modern_get_dmb_shm_id(). get_dmb_shm_id reports the shared
memory id of the region that holds the buffer.

Link: https://lore.kernel.org/virtio-comment/20260818060255.6853-1-graf@amazon.com/

> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 565d37b630b31..c893e31e51590 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c

[ ... ]

> @@ -364,6 +364,29 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
>  	}
>  }
>  
> +/*
> + * A device reports the memory type of its Device Memory Buffer region in
> + * dmb_mem_type, and the field is valid as soon as the device offers
> + * VIRTIO_F_DMB.  VIRTIO_DMB_MEM_TYPE_COHERENT says that a write by either side
> + * becomes visible to the other with no cache maintenance by the driver, which
> + * is what plain loads and stores on the region need.  A common configuration
> + * structure too short to hold the field reports no type at all, and reading it
> + * there would fall outside what vp_modern_probe() mapped.  This is the only
> + * place that measures the structure against the field, because the read
> + * happens before vp_check_common_size() runs.
> + */
> +static bool vp_dmb_mem_type_supported(struct virtio_pci_device *vp_dev)
> +{
> +	size_t need = offsetofend(struct virtio_pci_modern_common_cfg,
> +				  dmb_mem_type);
> +
> +	if (vp_dev->mdev.common_len < need)
> +		return false;
> +
> +	return vp_modern_get_dmb_mem_type(&vp_dev->mdev) ==

[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check
rely on an inflated common_len that could allow out-of-bounds MMIO reads?

If a buggy or malicious PCI device sets the capability length to 68 or more,
but places it near the end of the BAR such that the remaining space is only
between 56 (minlen) and 67 bytes, vp_modern_map_capability() sets
mdev->common_len to the un-clamped capability length:

drivers/virtio/virtio_pci_modern_dev.c:vp_modern_map_capability() {
    ...
    if (length > size)
        length = size;

    if (len)
        *len = length; /* mdev->common_len receives the inflated length */

    if (minlen + offset < minlen ||
        minlen + offset > pci_resource_len(dev, bar)) {
    ...
}

Since pci_iomap_range() will only map up to the physical end of the BAR,
the actual mapped region is shorter than common_len.

Because common_len is inflated, the check (vp_dev->mdev.common_len < need)
passes here, allowing vp_modern_get_dmb_mem_type() to perform a read
which is physically outside the mapped PCI BAR.

Could this unmapped read trigger a PCIe Unsupported Request and Master Abort,
potentially panicking the host system?

> +	       VIRTIO_DMB_MEM_TYPE_COHERENT;
> +}
> +
>  static void vp_transport_features(struct virtio_device *vdev, u64 features)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818211425.91009-1-graf@amazon.com?part=11