Re: [PATCH v6 4/6] vfio/pci: Add PCIe TPH enable/disable support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect `argsz` validation in `vfio_pci_tph_enable()` incorrectly adds the structure size twice, breaking the UAPI.
- [High] Missing locking in `vfio_pci_ioctl_tph()` leads to config space and `pci_dev` bitfield data corruption.
- [High] Disabling TPH after registering the device exposes it to userspace races.
- [High] TPH state is not explicitly disabled when the VFIO device is closed, leading to state inconsistencies and subsequent failures.
--

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

vfio/pci: Add PCIe TPH enable/disable support

This patch adds support to enable and disable the TPH function with mode
selection. It restricts the unsafe device-specific TPH mode via a module
parameter and disables TPH when taking over device ownership.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index d98d04bad4a39..69f666d20c4af 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -1492,6 +1492,43 @@ static int vfio_pci_tph_get_cap(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_tph_enable(struct vfio_pci_core_device *vdev,
> +			      struct vfio_device_pci_tph_op *op,
> +			      void __user *uarg)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	struct vfio_pci_tph_ctrl ctrl;
> +	int mode;
> +
> +	if (op->argsz < offsetofend(struct vfio_device_pci_tph_op, ctrl) +
> +			sizeof(struct vfio_pci_tph_ctrl))
> +		return -EINVAL;

Since offsetofend(TYPE, MEMBER) already includes the size of the ctrl member,
does adding sizeof(struct vfio_pci_tph_ctrl) a second time require more
bytes than the actual structure size?

Could this cause the check to reject valid userspace calls that provide the
exact structure size?

> +
> +	if (copy_from_user(&ctrl, uarg, sizeof(ctrl)))
> +		return -EFAULT;
> +
> +	if (ctrl.mode != VFIO_PCI_TPH_MODE_IV &&
> +	    ctrl.mode != VFIO_PCI_TPH_MODE_DS)
> +		return -EINVAL;
> +
> +	if (ctrl.mode == VFIO_PCI_TPH_MODE_DS && !enable_unsafe_tph_ds_mode)
> +		return -EOPNOTSUPP;
> +
> +	/* Reserved must be zero */
> +	if (memchr_inv(ctrl.reserved, 0, sizeof(ctrl.reserved)))
> +		return -EINVAL;
> +
> +	mode = (ctrl.mode == VFIO_PCI_TPH_MODE_IV) ? PCI_TPH_ST_IV_MODE :
> +						     PCI_TPH_ST_DS_MODE;
> +	return pcie_enable_tph(pdev, mode);
> +}

If userspace enables TPH but crashes or closes the file descriptor without
manually disabling it, is the TPH state left enabled since the release path
does not call pcie_disable_tph()?

If the device is opened again, could subsequent attempts to enable TPH fail
with -EBUSY because pcie_enable_tph() sees it already enabled?

> +
> +static int vfio_pci_tph_disable(struct vfio_pci_core_device *vdev)
> +{
> +	pcie_disable_tph(vdev->pdev);
> +	return 0;
> +}
> +
>  static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,
>  			      void __user *uarg)
>  {
> @@ -1504,6 +1541,10 @@ static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,
>  	switch (op.op) {
>  	case VFIO_PCI_TPH_GET_CAP:
>  		return vfio_pci_tph_get_cap(vdev, &op, uarg + minsz);
> +	case VFIO_PCI_TPH_ENABLE:
> +		return vfio_pci_tph_enable(vdev, &op, uarg + minsz);
> +	case VFIO_PCI_TPH_DISABLE:
> +		return vfio_pci_tph_disable(vdev);

Because vfio_pci_ioctl_tph() handles these ioctls locklessly without
holding vdev->igate, does concurrently calling pcie_enable_tph() cause a
data race on the pdev->tph_enabled bitfield?

Could this concurrently overwrite adjacent critical bitfields in
struct pci_dev like msi_enabled or is_managed?

>  	default:
>  		/* Other ops are not implemented yet */
>  		return -EINVAL;

[ ... ]

> @@ -2259,6 +2300,8 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>  	ret = vfio_register_group_dev(&vdev->vdev);
>  	if (ret)
>  		goto out_power;
> +	/* Disable TPH when taking over ownership of the device */
> +	pcie_disable_tph(pdev);

Is it safe to call pcie_disable_tph() after vfio_register_group_dev()?

Since the device becomes visible to userspace once registered, could a
userspace process instantly open the device and enable TPH, racing with this
initialization thread and overwriting the user's configuration?

Should hardware state initialization happen before publishing the device to
userspace?

>  	return 0;
>  
>  out_power:

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