Re: [PATCH v2 2/7] vfio: Add qcom_vfio.h header for MSI cache protocol

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pci: hardware state overwrite via pci_find_free_ext_cap_offset()
- [High] pci: out-of-bounds config space read in qcom_vfio_find_msi_cache
- [High] pci/vfio: false positive in linear MSI cache scan risks MSI misconfiguration
--

commit a4701d60f47321f6e79355d9a8e74d4e1c2789b7
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

vfio: Add qcom_vfio.h header for MSI cache protocol

Add a shared header for the qcom-vfio-pci driver and Qualcomm device drivers.
It defines a communication protocol for caching host MSI values in extended
PCI config space with a "QMSI" magic signature for VM discovery. This avoids
offset mismatches when QEMU filters certain capabilities.

> diff --git a/include/linux/qcom_vfio.h b/include/linux/qcom_vfio.h
> new file mode 100644
> index 0000000000000..7553bc382c8e4
> --- /dev/null
> +++ b/include/linux/qcom_vfio.h
> @@ -0,0 +1,90 @@

[ ... ]

> +static inline int qcom_vfio_find_msi_offset(struct pci_dev *pdev)
> +{
> +	return pci_find_free_ext_cap_offset(pdev, QCOM_VFIO_MSI_SPACE_SIZE);
> +}

[Severity: High]
Does relying on pci_find_free_ext_cap_offset() risk hardware state overwrite?

The implementation of pci_find_free_ext_cap_offset() does not skip the size
of the last extended capability. It begins scanning for zeroed registers
immediately at the start of the last active capability. Consequently, it can
misidentify zeroed registers (such as reserved or masked fields) inside
active capabilities as free space.

Using this offset to cache MSI values could overwrite live hardware
registers, causing a regression due to hardware state corruption.
Furthermore, if the extended config space is completely empty, it
incorrectly returns 0 instead of 0x100.

[ ... ]

> +static inline int qcom_vfio_find_msi_cache(struct pci_dev *pdev)
> +{
> +	u32 magic;
> +	int offset;
> +
> +	for (offset = PCI_CFG_SPACE_SIZE;
> +	     offset <= PCI_CFG_SPACE_EXP_SIZE - QCOM_VFIO_MSI_SPACE_SIZE;
> +	     offset += 4) {

[Severity: High]
Could this loop lead to out-of-bounds config space reads on bare-metal
systems?

The qcom_vfio_find_msi_cache() helper is called directly by the ath11k and
ath12k drivers during normal probe, meaning it executes on bare-metal
systems, not just inside VMs. The loop unconditionally reads up to
PCI_CFG_SPACE_EXP_SIZE (4096 bytes), completely bypassing the dynamic
pdev->cfg_size limit.

The PCI core generic pci_read_config_dword() does not perform bounds
checking against pdev->cfg_size and relies entirely on callers to respect
the boundaries. If the host platform restricts the config space size (for
example, due to a broken ECAM falling back to legacy accesses or a
PCIe-to-PCI bridge), this loop will force reads beyond safe boundaries.

Accessing undefined extended configuration space on bare-metal can cause
regressions like system lockups or fatal PCIe Unsupported Request panics.

> +		if (pci_read_config_dword(pdev, offset, &magic))
> +			continue;
> +		if (magic == QCOM_VFIO_MSI_MAGIC)
> +			return offset;
> +	}

[Severity: High]
Is it possible for this linear scan to falsely identify unrelated data as
the MSI cache, leading to misconfiguration?

This linear scan bypasses the PCIe standard capability chain traversal and
blindly reads all extended config registers, including dynamic logs like AER.

If a hardware error occurs and the AER Header Log captures a TLP containing
the value 0x49534D51, the scan will falsely identify it as the VFIO MSI
cache. It would then read the adjacent AER log values as msi_addr_lo,
msi_addr_hi, and msi_data, and program the physical device to use a garbage
MSI address.

This could lead to regressions including data corruption, IOMMU faults, and
loss of network interrupts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011074514.515894-1-jtornosm@redhat.com?part=2