Re: [PATCH v10 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 4 potential issue(s) to consider:
- [High] Memory leak on copy_to_user failure in the GET operation path.
- [High] Concurrent VFIO_DEVICE_FEATURE_TPH_ST ioctls lack serialization, leading to data races and PCI specification violations.
- [Medium] Flag-gated syscall parameter 'index' is validated outside of its flag scope.
- [Medium] Incorrect evaluation of VFIO_DEVICE_FEATURE_PROBE requests causes false feature reporting.
--

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

vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST for TPH ST entry management

This commit adds the VFIO_DEVICE_FEATURE_TPH_ST feature to allow userspace
to manage PCIe TPH Steering Tag entries. It provides both SET and GET
operations depending on the capabilities of the hardware.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index cc13fc8eea9da..13304a37ba266 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1521,6 +1521,93 @@ 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 __user *arg,
> +					size_t argsz)
> +{
> +	bool is_set = !!(flags & VFIO_DEVICE_FEATURE_SET);
> +	struct vfio_device_feature_tph_st tph_st;
> +	struct pci_dev *pdev = vdev->pdev;
> +	enum tph_mem_type mtype;
> +	int i, j, ret;
> +	u32 *cpus;
> +	u16 st;
> +
> +	if (!enable_unsafe_tph ||
> +		pcie_tph_enabled_mode(pdev) == PCI_TPH_ST_NS_MODE)
> +		return -EOPNOTSUPP;
> +	if (!is_set && pcie_tph_enabled_mode(pdev) != PCI_TPH_ST_DS_MODE)
> +		return -EOPNOTSUPP;
> +	if (is_set && pcie_tph_get_st_table_loc(pdev) == PCI_TPH_LOC_NONE)
> +		return -EOPNOTSUPP;

Will these capability checks improperly handle VFIO_DEVICE_FEATURE_PROBE requests?

If userspace probes with a bare VFIO_DEVICE_FEATURE_PROBE, is_set is false.
This would reject the probe with -EOPNOTSUPP if the device is not in DS mode,
falsely reporting that the entire feature is unsupported even if SET is
supported.

Conversely, if userspace probes with PROBE | GET | SET, is_set is true, which
bypasses the GET-specific DS mode check, falsely reporting that GET operations
are supported.

> +
> +	ret = vfio_check_feature(flags, argsz,
> +				 VFIO_DEVICE_FEATURE_GET |
> +				 VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(tph_st));
> +	if (ret <= 0)
> +		return ret;
> +
> +	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 != 0)
> +		return -EINVAL;

Does this validation evaluate a parameter outside of its flag scope?

The UAPI documentation states that the index field is only valid for
FEATURE_SET. Since userspace is not required to zero-fill unused arguments,
this might cause valid GET requests to spuriously fail with -EINVAL if the
unused index field happens to contain a non-zero value.

> +	if (is_set && (tph_st.index >= VFIO_TPH_ST_MAX_COUNT ||
> +		       tph_st.index + tph_st.count > VFIO_TPH_ST_MAX_COUNT))
> +		return -EINVAL;
> +
> +	cpus = memdup_array_user(&arg->data, tph_st.count, sizeof(*cpus));
> +	if (IS_ERR(cpus))
> +		return PTR_ERR(cpus);
> +
> +	mtype = tph_st.flags & VFIO_TPH_ST_MEM_TYPE_PM ? TPH_MEM_TYPE_PM :
> +							 TPH_MEM_TYPE_VM;
> +	if (!is_set) {
> +		for (i = 0; i < tph_st.count; i++) {
> +			ret = pcie_tph_get_cpu_st(pdev, mtype, cpus[i], &st);
> +			if (ret)
> +				goto out;
> +			cpus[i] = st;
> +		}
> +		goto out;
> +	}
> +
> +	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);

Are concurrent ioctls serialized here?

There appears to be no locking (such as vdev->igate) when iterating and
calling pcie_tph_set_st_entry().

Concurrent SET operations could race and cause lost updates to the PCI
config space control registers, or cause the error path rollback to blindly
overwrite successful entries programmed by a racing thread.

> +			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) {
> +		if (copy_to_user(&arg->data, cpus,
> +			tph_st.count * sizeof(*cpus)))
> +			return -EFAULT;

Does this early return leak the cpus array?

If copy_to_user() fails, the code returns -EFAULT directly and bypasses
the kfree(cpus) cleanup.

A buggy or malicious userspace application could repeatedly issue GET
requests with an unmapped data pointer, leading to memory exhaustion.

> +	}
> +	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);
> +	}
> +	kfree(cpus);
> +	return ret;
> +}

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