Re: [PATCH v11 4/5] vfio/pci: Add PCIe TPH configuration space virtualization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Unsafe PCI configuration space access during PM state transition (Race condition leading to Unsupported Request).
- [High] Dropped configuration writes to the TPH capability break ST Table and Control Register virtualization.
- [High] Host kernel API overrides the guest's explicit TPH Request Enable state, preventing the guest from disabling TPH requests.
--

commit 5ada31d58b6a34d641ac5392c15e54c3404870a2
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add PCIe TPH configuration space virtualization

This commit adds support for virtualizing the PCIe TPH control register.
It introduces a module parameter to globally control the TPH feature and
mediates TPH control register writes to only allow valid mode settings.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
[ ... ]
> @@ -313,6 +316,30 @@ static int vfio_virt_config_read(struct vfio_pci_core_device *vdev, int pos,
>  	return count;
>  }
>  
> +static int vfio_pci_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
> +				     int count, struct perm_bits *perm,
> +				     int offset, __le32 val)
> +{
> +	u32 data = le32_to_cpu(val);
> +
> +	guard(mutex)(&vdev->tph_lock);
> +
> +	if (!enable_unsafe_tph)
> +		return count;
> +
> +	if (offset != PCI_TPH_CTRL)
> +		return count;

Does this function silently discard ST Table and other TPH capability writes?

By unconditionally returning count without actually modifying the virtual
configuration space (vconfig) or the physical hardware, guest writes to the ST
Table, the ST Table Select field, or unaligned byte-writes to the control
register are dropped.

Since there is no .readfn, reads will fetch directly from the physical hardware
returning unconfigured values, which could break guest OS configurations.

> +
> +	/* Only permit write TPH mode. */
> +	data &= PCI_TPH_CTRL_MODE_SEL_MASK;
> +	if (data == PCI_TPH_ST_IV_MODE || data == PCI_TPH_ST_DS_MODE)
> +		pcie_enable_tph(vdev->pdev, data);

If the guest clears the 'TPH Requester Enable' field but leaves a valid mode
in PCI_TPH_CTRL_MODE_SEL_MASK, does this call to pcie_enable_tph() override
the guest's request?

pcie_enable_tph() force-enables the feature in physical hardware, which prevents
the guest from disabling it.

Additionally, if the guest changes active modes, pcie_enable_tph() will return
-EBUSY and fail silently without informing the user.

> +	else if (data == PCI_TPH_ST_NS_MODE)
> +		pcie_disable_tph(vdev->pdev);
> +
> +	return count;
> +}
> +
>  static struct perm_bits direct_ro_perms = {
[ ... ]
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -736,6 +738,11 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>  #endif
>  	vfio_pci_dma_buf_cleanup(vdev);
>  
> +	/* Disable TPH when userspace closes the device FD */
> +	mutex_lock(&vdev->tph_lock);
> +	pcie_disable_tph(vdev->pdev);
> +	mutex_unlock(&vdev->tph_lock);
> +
>  	vfio_pci_core_disable(vdev);

Is it safe to access the PCI config space here?

A malicious or buggy guest could have left the device in the suspended D3 state.

vfio_pci_core_disable() wakes up the device via pm_runtime_resume(), but
calling pcie_disable_tph() before that could access a suspended device. This
would lead to an Unsupported Request (UR) and potentially a host kernel panic.

[ ... ]
> @@ -2205,6 +2214,9 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>  	if (!disable_idle_d3)
>  		pm_runtime_put(dev);
>  
> +	/* Disable TPH when taking over ownership of the device */
> +	pcie_disable_tph(pdev);
> +

Could calling pcie_disable_tph() after pm_runtime_put() risk accessing a
suspended device?

pm_runtime_put() drops the runtime PM reference, allowing the PM core to
asynchronously suspend the device to D3. Accessing the config space of a
suspended device violates PCIe semantics.

>  	ret = vfio_register_group_dev(&vdev->vdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518071701.25177-1-fengchengwen@huawei.com?part=4