Re: [PATCH v11 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 2 potential issue(s) to consider:
- [Medium] The function mixes `guard()`-based cleanup with a `goto`-based cleanup label.
- [Medium] The flag-gated parameter `index` is validated outside the scope of its flag gate.
--

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

vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST for TPH ST entry management

This commit introduces VFIO_DEVICE_FEATURE_TPH_ST to allow userspace to manage
PCIe TPH Steering Tag entries. It supports SET operations to program contiguous
ST entries and GET operations to retrieve ST values per CPU ID.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> +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;
> +
> +	guard(mutex)(&vdev->tph_lock);
[ ... ]
> +	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;

Since the index field is documented as only valid for FEATURE_SET operations,
could userspace legitimately pass uninitialized memory in this field during
a GET request?

If userspace is not required to zero-fill unused fields, validating that
index is exactly 0 during a GET operation might cause spurious failures
for valid requests.

[ ... ]
> +	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;

Is it recommended to mix guard() based cleanup with goto-based error handling
in the same function?

The cleanup guidelines suggest that functions should either use scope-based
cleanups for all resources or convert none of them. Could the cpus array be
managed with __free(kfree) and the rollback handled directly in the error
paths to avoid using goto labels alongside guard()?

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