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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Concurrent userspace config writes cause data races that corrupt struct pci_dev bitfields.
- [High] pcie_disable_tph() is called on a suspended device, causing invalid PCI configuration space accesses.
- [High] Returning error codes from the config write function fails the `pwrite` syscall, breaking VM migration and restoration.
- [High] Idempotent or read-modify-write operations on the TPH Control Register by the guest fail with a fatal error.
- [Medium] TPH mediation logic completely ignores the guest's requested Requester Enable state.
--

commit 273d5243ed79b3842db13976044c596f4d95a83f
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add PCIe TPH configuration space virtualization

Add support for virtualizing the PCIe TPH (Transaction Processing Hints)
control register. TPH may break platform isolation, so add a module
parameter "enable_unsafe_tph" to allow administrators to globally control
this feature.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a10ed733f0e3a..efb413ce78170 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c

[ ... ]

> @@ -313,6 +316,35 @@ 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);
> +	int ret;
> +
> +	if (!enable_unsafe_tph)
> +		return -EOPNOTSUPP;
> +
> +	if (count != 4 || offset != PCI_TPH_CTRL)
> +		return -EINVAL;

Will returning negative error codes like -EOPNOTSUPP or -EINVAL here propagate
to the pwrite() syscall and abort VM migration? 

During state restoration, hypervisors typically restore the entire config
space, including read-only and unsupported capability fields. Should these
writes be silently dropped by returning count instead?

> +
> +	/* Only permit write TPH mode. */
> +	data &= PCI_TPH_CTRL_MODE_SEL_MASK;

Does this logic ignore the guest's requested Requester Enable state?

The TPH Requester Enable field is in bits 8-9 of the TPH Control Register,
but this masks the data using the ST Mode mask (bits 0-2). If a guest OS
tries to disable TPH by clearing the Requester Enable bits while leaving ST
Mode unchanged, won't this mistakenly see the non-zero mode and call
pcie_enable_tph()?

> +	if (data > PCI_TPH_ST_DS_MODE)
> +		return -EINVAL;
> +
> +	if (data == PCI_TPH_ST_NS_MODE) {
> +		pcie_disable_tph(vdev->pdev);
> +		return 4;
> +	}
> +
> +	ret = pcie_enable_tph(vdev->pdev, data);

Can concurrent userspace config writes cause data races here? 

Since this is invoked during the pwrite system call without holding a
device-level lock, concurrent calls to pcie_enable_tph() or
pcie_disable_tph() will perform unprotected read-modify-write operations on
pdev->tph_enabled. 

As tph_enabled is a C bitfield adjacent to other critical bitfields like
msi_enabled, could this clobber unrelated PCI features?

> +	if (ret)
> +		return -EIO;

Could this cause a fatal error if the guest OS performs an idempotent write?

If the guest OS performs a read-modify-write on the PCI_TPH_CTRL register
when TPH is already active, pcie_enable_tph() returns -EBUSY. Returning -EIO
here would propagate to userspace and likely crash the VM.

> +	return 4;
> +}

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8a..cc13fc8eea9da 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -736,6 +738,9 @@ 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 */
> +	pcie_disable_tph(vdev->pdev);
> +
>  	vfio_pci_core_disable(vdev);

Is it safe to call pcie_disable_tph() here before vfio_pci_core_disable()
wakes the device? 

If the device is currently in a suspended low-power state (D3), calling
pcie_disable_tph() will write to the config space, which could cause
Unsupported Request (UR) Master Aborts on the PCIe bus and trigger AER
faults.

>  
>  	mutex_lock(&vdev->igate);

[ ... ]

> @@ -2205,6 +2210,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);
> +
>  	ret = vfio_register_group_dev(&vdev->vdev);

Could this also access the config space of a suspended device?

pm_runtime_put() is called right before this, which allows the device to
suspend. Writing to the TPH Control Register while the device is in D3
would violate physical state machine constraints.

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