Re: [PATCH 3/7] vfio/pci: Add qcom-vfio-pci variant driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] vfio/pci: TOCTOU in ioctl double fetch leads to NULL pointer dereference
- [High] vfio/pci: hardware corruption via writes to unassigned config space
- [High] vfio/pci: stale MSI cache across VM reboots due to missing state reset
- [Medium] vfio/pci: data truncation and swapped bytes on big-endian hosts
- [Medium] vfio/pci: sub-dword reads bypass MSI cache and return hardware garbage
- [Medium] vfio/pci: data race in MSI cache updates allows reading uninitialized values
--

commit 6afc81e503658e11a186e8970fd3ac931221f37d
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Subject: vfio/pci: Add qcom-vfio-pci variant driver

Add a VFIO variant driver for Qualcomm PCIe devices (ath11k, ath12k) that
caches physical host MSI addresses and passes them through via extended
config space.

> --- /dev/null
> +++ b/drivers/vfio/pci/qcom/main.c
[ ... ]
> +static ssize_t qcom_vfio_pci_read(struct vfio_device *core_vdev, char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
[ ... ]
> +	if (index == VFIO_PCI_CONFIG_REGION_INDEX) {
> +		ret = qcom_vfio_pci_config_rw(qdev, buf, count, offset, false);
> +		if (ret != -ENOTTY) {
> +			if (ret > 0)
> +				*ppos += ret;
> +			return ret;
> +		}
> +	}
> +
> +	return vfio_pci_core_read(core_vdev, buf, count, ppos);

[Severity: Medium]
Will this fallback to vfio_pci_core_read() leak hardware garbage to the guest
if a sub-dword read bypasses the MSI cache interception? Since
qcom_intercept_msi_read() strictly matches exact 4-byte or 2-byte sizes,
unaligned or smaller guest reads will fall through and return -ENOTTY, causing
a real physical read from the unbacked capability gap.

[ ... ]
> +static long qcom_vfio_pci_ioctl(struct vfio_device *core_vdev,
> +				unsigned int cmd, unsigned long arg)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +	struct qcom_vfio_pci_device *qdev = to_qcom_vdev(vdev);
> +	struct vfio_irq_set hdr;
> +	long ret;
> +
> +	ret = vfio_pci_core_ioctl(core_vdev, cmd, arg);
> +
> +	/*
> +	 * If SET_IRQS ioctl succeeded for MSI and MSI wasn't cached yet,
> +	 * cache the MSI descriptor values.
> +	 * Only intercept MSI index (not INTx, MSI-X, ERR, or REQ).
> +	 */
> +	if (ret == 0 && cmd == VFIO_DEVICE_SET_IRQS) {
> +		unsigned long minsz = offsetofend(struct vfio_irq_set, count);
> +
> +		if (copy_from_user(&hdr, (void __user *)arg, minsz))

[Severity: Critical]
Can this double fetch of the ioctl argument from userspace create a race
condition leading to a NULL pointer dereference?

The copy_from_user() happens after vfio_pci_core_ioctl() has already
processed the buffer. A concurrent thread could rewrite hdr.index in userspace
to VFIO_PCI_MSI_IRQ_INDEX after the core validates it. This would cause the
driver to execute qcom_cache_and_write_msi(), which takes the MSI descriptor
lock (__msi_lock_descs(dev)) on an unallocated dev->msi.data, resulting in
a kernel panic.

> +			return ret;
> +
> +		if (hdr.index == VFIO_PCI_MSI_IRQ_INDEX) {
> +			pci_dbg(qdev->vdev.pdev,
> +				 "SET_IRQS: index=%u flags=0x%x start=%u count=%u cached=%d\n",
> +				 hdr.index, hdr.flags, hdr.start, hdr.count, qdev->msi_cached);
> +
> +			if (!qdev->msi_cached && (hdr.flags & VFIO_IRQ_SET_ACTION_TRIGGER))
> +				qcom_cache_and_write_msi(qdev);

[Severity: High]
Does skipping the MSI cache update here break interrupt delivery if a VM is
rebooted or a new VM attaches to the same device?

Since qdev->msi_cached is initialized to false in qcom_vfio_pci_init_dev()
but never reset to false when the device is closed (e.g., in
qcom_vfio_pci_ops), a subsequent VM attachment will see !qdev->msi_cached as
false. The driver would then silently fail to cache the new MSI allocations,
leaving the guest with stale MSI addresses from the previous VM execution.

> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct vfio_device_ops qcom_vfio_pci_ops = {
> +	.name		= "qcom-vfio-pci",
> +	.init		= qcom_vfio_pci_init_dev,
> +	.release	= vfio_pci_core_release_dev,
> +	.open_device	= qcom_vfio_pci_open_device,
> +	.close_device	= vfio_pci_core_close_device,

[Severity: High]
This appears to be where a wrapper around vfio_pci_core_close_device would be
needed to clear qdev->msi_cached back to false when a VM shuts down,
preventing the stale MSI cache issue mentioned above.

[ ... ]
> --- /dev/null
> +++ b/drivers/vfio/pci/qcom/msi_quirk.c
> @@ -0,0 +1,163 @@
[ ... ]
> +void qcom_cache_and_write_msi(struct qcom_vfio_pci_device *qdev)
> +{
> +	struct pci_dev *pdev = qdev->vdev.pdev;
> +	struct device *dev = &pdev->dev;
> +	struct msi_desc *desc;
> +	u32 addr_lo, addr_hi;
> +	u16 data;
> +
> +	if (qdev->msi_cached)
> +		return;

[Severity: High]
This early return relies on qdev->msi_cached, which appears to never be
reset across VM reboots, as noted in qcom_vfio_pci_ioctl().

[ ... ]
> +	qdev->msi_addr_lo = addr_lo;
> +	qdev->msi_addr_hi = addr_hi;
> +	qdev->msi_data = data;
> +	qdev->msi_cached = true;

[Severity: Medium]
Could this lack of a write memory barrier before setting qdev->msi_cached
allow a concurrent reader in qcom_intercept_msi_read() to observe
msi_cached == true but read stale or zeroed MSI addresses?

> +
> +	pci_write_config_dword(pdev, qdev->msi_offset + QCOM_VFIO_MSI_MAGIC_OFFSET,
> +			       QCOM_VFIO_MSI_MAGIC);
> +	pci_write_config_dword(pdev, qdev->msi_offset + QCOM_VFIO_MSI_ADDR_LO_OFFSET,
> +			       qdev->msi_addr_lo);
> +	pci_write_config_dword(pdev, qdev->msi_offset + QCOM_VFIO_MSI_ADDR_HI_OFFSET,
> +			       qdev->msi_addr_hi);
> +	pci_write_config_word(pdev, qdev->msi_offset + QCOM_VFIO_MSI_DATA_OFFSET,
> +			      qdev->msi_data);

[Severity: High]
Are these physical writes to unassigned configuration space safe on PCIe
hardware?

qdev->msi_offset points to a free capability gap discovered by
pci_find_free_ext_cap_offset(). PCIe endpoints are not required to implement
registers in unused gaps, so writing to them using pci_write_config_dword()
violates the PCIe specification and risks causing Unhandled Request (UR) faults
that can trigger AER cascades and crash the host.

Given that guest reads are emulated by the driver in
qcom_intercept_msi_read(), do these values actually need to be physically
written to the hardware?

[ ... ]
> +static ssize_t qcom_intercept_msi_read(struct qcom_vfio_pci_device *qdev,
> +				       loff_t pos, char __user *buf, size_t count)
> +{
> +	u32 tmp_val = 0;
> +	int msi_magic, msi_addr_lo, msi_addr_hi, msi_data;
> +
> +	if (!qdev->msi_offset)
> +		return -ENOTTY;
> +
> +	msi_magic = qdev->msi_offset + QCOM_VFIO_MSI_MAGIC_OFFSET;
> +	msi_addr_lo = qdev->msi_offset + QCOM_VFIO_MSI_ADDR_LO_OFFSET;
> +	msi_addr_hi = qdev->msi_offset + QCOM_VFIO_MSI_ADDR_HI_OFFSET;
> +	msi_data = qdev->msi_offset + QCOM_VFIO_MSI_DATA_OFFSET;
> +
> +	if (pos == msi_magic && count == sizeof(u32)) {

[Severity: Medium]
Will this strict size and position check cause sub-dword guest reads (e.g., a
1-byte read) to fall through and bypass the interception entirely? This relates
to the fallback in qcom_vfio_pci_read() returning hardware garbage.

> +		if (qdev->msi_cached) {
> +			tmp_val = QCOM_VFIO_MSI_MAGIC;
> +			pci_dbg(qdev->vdev.pdev, "Intercepting MAGIC read at 0x%llx, returning 0x%x (QMSI)\n",
> +				 pos, tmp_val);
> +		} else {
> +			return -ENOTTY;
> +		}
> +	} else if (qdev->msi_cached) {

[Severity: Medium]
Does evaluating qdev->msi_cached without a read memory barrier expose the
driver to race conditions, as mentioned in qcom_cache_and_write_msi()?

> +		if (pos == msi_addr_lo && count == sizeof(u32)) {
> +			tmp_val = qdev->msi_addr_lo;
> +			pci_dbg(qdev->vdev.pdev, "Intercepting ADDR_LO read at 0x%llx, returning 0x%x\n",
> +				 pos, tmp_val);
> +		} else if (pos == msi_addr_hi && count == sizeof(u32)) {
> +			tmp_val = qdev->msi_addr_hi;
> +			pci_dbg(qdev->vdev.pdev, "Intercepting ADDR_HI read at 0x%llx, returning 0x%x\n",
> +				 pos, tmp_val);
> +		} else if (pos == msi_data && count == sizeof(u16)) {
> +			tmp_val = qdev->msi_data;

[Severity: Medium]
Will this zero-extension of the 16-bit msi_data into the 32-bit tmp_val
cause the subsequent copy_to_user of sizeof(u16) bytes to return zeroes
instead of the actual MSI data on big-endian platforms?

> +			pci_dbg(qdev->vdev.pdev, "Intercepting MSI_DATA read at 0x%llx, returning 0x%x\n",
> +				 pos, tmp_val);
> +		} else {
> +			return -ENOTTY;
> +		}
> +	} else {
> +		return -ENOTTY;
> +	}
> +
> +	if (copy_to_user(buf, &tmp_val, count))

[Severity: Medium]
Since PCI config space requires a little-endian presentation, does copying the
host-endian tmp_val directly to userspace here without endianness conversion
truncate and byte-swap the MSI values on big-endian hosts?

> +		return -EFAULT;
> +
> +	return count;
> +}

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