Re: [PATCH v9 5/5] vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST for TPH ST entry management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The function validates the flag-gated `index` parameter during GET operations.
- [High] The return value of `copy_to_user` is returned directly to userspace instead of being translated to `-EFAULT`.
- [High] Feature probing is broken because `vfio_pci_core_feature_tph_st` incorrectly returns `-EINVAL` for a valid PROBE request.
- [High] Unprotected multi-entry loop and rollback allows concurrent ioctls to corrupt the device's Steering Tag table.
- [Low] Missing `__user` annotation on the `arg` pointer parameter.
--

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

vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST for TPH ST entry management

This commit introduces a new VFIO device feature allowing userspace
to manage PCIe TPH Steering Tag entries. It supports set operations
to program contiguous entries and get operations to retrieve ST values.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index cc13fc8eea9da..298e7dd136fd2 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1521,6 +1521,91 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_core_feature_tph_st(struct vfio_pci_core_device *vdev,
> +					u32 flags,
> +					struct vfio_device_feature_tph_st *arg,

This isn't a bug, but should arg be annotated with __user to satisfy
sparse static analysis? The caller passes a void __user pointer, and
dropping the annotation circumvents kernel address-space checks.

> +					size_t argsz)
> +{
> +	bool is_set = !!(flags & VFIO_DEVICE_FEATURE_SET);

[ ... ]

> +	ret = vfio_check_feature(flags, argsz,
> +				 VFIO_DEVICE_FEATURE_GET |
> +				 VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(tph_st));
> +	if (ret != 1)
> +		return -EINVAL;

Will this break feature probing? When VFIO_DEVICE_FEATURE_PROBE
is requested, vfio_check_feature() validates the request and returns 0.
By converting any return value other than 1 into -EINVAL, userspace
might be unable to successfully probe for VFIO_DEVICE_FEATURE_TPH_ST
support. Should this return ret instead of -EINVAL?

> +
> +	if (copy_from_user(&tph_st, arg, sizeof(tph_st)))
> +		return -EFAULT;
> +
> +	if (tph_st.count == 0 || tph_st.count > VFIO_TPH_ST_MAX_COUNT ||
> +		tph_st.flags > VFIO_TPH_ST_MEM_TYPE_PM)
> +		return -EINVAL;
> +	if (is_set && (tph_st.index >= VFIO_TPH_ST_MAX_COUNT ||
> +		       tph_st.index + tph_st.count > VFIO_TPH_ST_MAX_COUNT))
> +		return -EINVAL;
> +	if (!is_set && tph_st.index != 0)
> +		return -EINVAL;

Since the UAPI documentation states index is only valid for
VFIO_DEVICE_FEATURE_SET, could validating it during a get request
incorrectly reject valid calls? Unused parameters might contain stack
garbage, so if userspace leaves index uninitialized during a get, this
forces them to zero-fill it unnecessarily.

[ ... ]

> +	for (i = 0; i < tph_st.count; i++) {
> +		if (cpus[i] == U32_MAX) {
> +			ret = pcie_tph_set_st_entry(pdev, tph_st.index + i, 0);
> +			if (ret)
> +				goto out;
> +			continue;
> +		}
> +
> +		ret = pcie_tph_get_cpu_st(pdev, mtype, cpus[i], &st);
> +		if (ret)
> +			goto out;
> +		ret = pcie_tph_set_st_entry(pdev, tph_st.index + i, st);
> +		if (ret)
> +			goto out;
> +	}
> +
> +out:
> +	if (!is_set && !ret)
> +		ret = copy_to_user(&arg->data, cpus,
> +				   tph_st.count * sizeof(*cpus));

Does this correctly propagate -EFAULT if the copy fails? copy_to_user()
returns the number of uncopied bytes, which is a positive integer.
Because userspace often interprets a non-negative ioctl return as
success, it might falsely assume the get operation succeeded while
operating on uninitialized or partially written data.

> +	if (is_set && ret) {
> +		/* Roll back previously programmed entries to 0 */
> +		for (j = 0; j < i; j++)
> +			pcie_tph_set_st_entry(pdev, tph_st.index + j, 0);
> +	}

Could concurrent ioctl calls corrupt the Steering Tag table? Since this
loop and rollback process does not appear to hold an overarching lock
(like vdev->igate), two overlapping set operations could race. If one
thread fails halfway through, its rollback loop might blindly overwrite
entries that the concurrent thread successfully programmed, leaving the
hardware in an inconsistent state.

> +	kfree(cpus);
> +	return ret;
> +}

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