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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect `argsz` size validation unconditionally rejects valid ioctl requests.
- [Medium] The unused `st` field in `struct vfio_pci_tph_entry` is not validated for being zero during SET_ST, breaking forward ABI compatibility.
--

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

vfio/pci: Add PCIe TPH SET_ST interface

This commit adds the VFIO_PCI_TPH_SET_ST operation to support batch
programming of steering tag entries. It also implements a rollback
mechanism to clear successfully programmed entries if any entry fails
to prevent inconsistent device state.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 45e641ab2a887..867d8694b56bd 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1600,6 +1600,90 @@ static int vfio_pci_tph_get_st(struct vfio_pci_core_device *vdev,
>  	return err;
>  }
>  
> +static int vfio_pci_tph_set_st(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_entry *ents;
> +	struct vfio_pci_tph_st st;
[ ... ]
> +	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;

Does adding sizeof(struct vfio_pci_tph_st) artificially inflate the required
buffer size?

The offsetofend() macro already includes the size of the st member. If a
properly written userspace application computes its argsz exactly as
sizeof(struct vfio_device_pci_tph_op) + size, this check would fail and
reject the request with -EINVAL.

> +
> +	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 < 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;
> +		}

During the SET_ST operation, the st field in struct vfio_pci_tph_entry
appears to be unused, as the steering tag is instead dynamically fetched
via pcie_tph_get_cpu_st() later in the loop.

Should the st field also be validated to be zero?

If userspace leaves this unused field uninitialized and passes garbage data,
it might break forward ABI compatibility. A future kernel update that attempts
to read explicit steering tags directly from this field wouldn't be able to
distinguish between old programs sending uninitialized garbage and new
programs sending valid data.

[ ... ]

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