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