Re: [PATCH v6 5/6] vfio/pci: Add PCIe TPH GET_ST interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `argsz` validation incorrectly double-counts the size of the union member, causing valid requests to be rejected.
- [High] Concurrent TPH ioctls lead to data races on unprotected PCIe TPH state variables and device configuration space.
- [Medium] The unused `index` field in `struct vfio_pci_tph_entry` is not validated to be zero during `VFIO_PCI_TPH_GET_ST`.
--

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

vfio/pci: Add PCIe TPH GET_ST interface

This commit adds support to batch get CPU steering tags for device-specific
TPH mode that does not implement an ST table. This interface requires
enabling the enable_unsafe_tph_ds_mode module parameter.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 69f666d20c4af..45e641ab2a887 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1529,6 +1529,77 @@ static int vfio_pci_tph_disable(struct vfio_pci_core_device *vdev)
>  	return 0;
>  }
>  
> +static int vfio_pci_tph_get_st(struct vfio_pci_core_device *vdev,
> +			       struct vfio_device_pci_tph_op *op,
> +			       void __user *uarg)
> +{
[ ... ]
> +	if (!st.count || st.count > VFIO_PCI_TPH_MAX_ENTRIES)
> +		return -EINVAL;
> +
> +	size = st.count * sizeof(*ents);
> +	if (op->argsz < offsetofend(struct vfio_device_pci_tph_op, st) +
> +			sizeof(struct vfio_pci_tph_st) + size)
> +		return -EINVAL;

Is it possible the size calculation double-counts
sizeof(struct vfio_pci_tph_st)?

The macro offsetofend(TYPE, MEMBER) evaluates to offsetof(TYPE, MEMBER) +
sizeof_field(TYPE, MEMBER). For the st member, offsetofend() correctly
calculates the end boundary. Adding sizeof(struct vfio_pci_tph_st) a
second time makes the expected minimum argsz larger than it should be.

If userspace passes the properly calculated size, will the kernel
incorrectly evaluate this as false and reject valid ioctl calls with -EINVAL?

(Note: The identical calculation error appears to be present in
vfio_pci_tph_enable() introduced in a prior patch.)

> +
> +	ents = kvmalloc(size, GFP_KERNEL);
> +	if (!ents)
> +		return -ENOMEM;
> +
> +	ents_off = offsetof(struct vfio_pci_tph_st, ents);
> +	if (copy_from_user(ents, uarg + ents_off, size)) {
> +		err = -EFAULT;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < st.count; i++) {
> +		/* Check reserved fields are zero */
> +		if (memchr_inv(&ents[i].reserved0, 0, sizeof(ents[i].reserved0)) ||
> +		    memchr_inv(&ents[i].reserved1, 0, sizeof(ents[i].reserved1))) {
> +			err = -EINVAL;
> +			goto out;
> +		}

Does this code need to validate that the unused index field in
struct vfio_pci_tph_entry is zero?

While reserved0 and reserved1 are checked, the index field appears to
be ignored during GET_ST operations. Because userspace is not required to
zero-fill unused arguments, this field may contain uninitialized garbage.

If a future kernel extension assigns meaning to index for GET_ST, will
this lack of validation break backwards compatibility by making it
impossible to distinguish intentional new values from uninitialized data in
older applications?

[ ... ]
>  static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,
>  			      void __user *uarg)
>  {
> @@ -1545,6 +1616,8 @@ static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,
>  		return vfio_pci_tph_enable(vdev, &op, uarg + minsz);
>  	case VFIO_PCI_TPH_DISABLE:
>  		return vfio_pci_tph_disable(vdev);
> +	case VFIO_PCI_TPH_GET_ST:
> +		return vfio_pci_tph_get_st(vdev, &op, uarg + minsz);
>  	default:
>  		/* Other ops are not implemented yet */
>  		return -EINVAL;

Are concurrent TPH ioctls adequately serialized here?

The vfio_pci_ioctl_tph() handler processes requests without acquiring any
serialization locks (such as vdev->igate). If userspace issues multiple
TPH ioctls concurrently, vfio_pci_tph_enable(), vfio_pci_tph_disable(),
and vfio_pci_tph_get_st() can execute in parallel.

The underlying PCI core functions pcie_enable_tph() and pcie_disable_tph()
perform lockless read-modify-write operations on the PCI_TPH_CTRL config
register and mutate pdev->tph_mode, pdev->tph_req_type, and
pdev->tph_enabled. Simultaneously, vfio_pci_tph_get_st() invokes
pcie_tph_get_cpu_st(), which relies on these state variables.

Can this lead to data races on unprotected PCIe TPH state variables,
resulting in corrupted device configuration space or inconsistent state
reads?

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