Re: [PATCH v12 5/6] vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST for PCIe TPH steering tag management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The return value of `copy_to_user()` is mishandled in `vfio_pci_tph_op_cpu_query()`, resulting in a positive return value to userspace on memory faults.
- [High] Hardware modifications to the ST table via MSI-X MMIO occur without ensuring the PCI device is in an active power state (e.g., D0), risking PCIe errors.
- [Low] Unconditional memory allocation for the TPH ST shadow table during device initialization, even when the TPH feature is globally disabled.
--

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

vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST for PCIe TPH steering tag management

Add new userspace device feature to access and maintain PCIe TPH Steering
Tag table entries, supporting two mutually exclusive operation modes:
raw table read/write and CPU ID lookup.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8a..8cf341689a478 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1516,6 +1518,159 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
[ ... ]
> +static int vfio_pci_tph_op_cpu_query(struct vfio_pci_core_device *vdev,
> +				     struct vfio_device_feature_tph_st *arg)
> +{
[ ... ]
> +	for (i = 0; i < arg->count; i++) {
> +		ret = pcie_tph_get_cpu_st(pdev, mtype, cpus[i], &st);
> +		if (ret)
> +			goto out;
> +		cpus[i] = st;
> +	}
> +
> +	ret = copy_to_user(uptr, cpus, arg->count * sizeof(u32));
> +out:
> +	kfree(cpus);
> +	return ret;

[Severity: High]
Does this return a positive value to userspace if the copy fails? 
copy_to_user() returns the number of bytes that failed to copy, so returning 
it directly here instead of a negative error code like -EFAULT might break 
the ioctl ABI, as userspace typically checks for negative error codes.

> +}
> +
> +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)
> +{
[ ... ]
> +	switch (op_type) {
> +	case VFIO_TPH_ST_OP_RAW_TABLE:
> +		if (is_set && is_get)
> +			return -EINVAL;
> +		return vfio_pci_tph_op_raw_table(vdev, is_get, &tph_st);

[Severity: High]
Is it possible for the device to be in a suspended power state (e.g., D3hot) 
when this ioctl is called? 

If userspace triggers this while suspended, the subsequent call to 
pcie_tph_set_st_entry() might perform direct MMIO writes to the MSI-X table.
Accessing MMIO on a suspended device can cause PCIe Unsupported Request (UR)
errors or Target Aborts. 

Should there be a pm_runtime_resume_and_get() or similar power state check 
before accessing hardware registers?

> +	case VFIO_TPH_ST_OP_CPU_QUERY:
> +		if (is_set)
> +			return -EOPNOTSUPP;
> +		return vfio_pci_tph_op_cpu_query(vdev, &tph_st);
> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> @@ -2097,12 +2254,23 @@ int vfio_pci_core_init_dev(struct vfio_device *core_vdev)
>  	mutex_init(&vdev->igate);
>  	spin_lock_init(&vdev->irqlock);
>  	mutex_init(&vdev->ioeventfds_lock);
> +	mutex_init(&vdev->tph_lock);
> +	vdev->tph_st_entries = vfio_pci_tph_st_shadow_size(vdev);
> +	vdev->tph_st_shadow = NULL;
> +	if (vdev->tph_st_entries) {
> +		vdev->tph_st_shadow = kcalloc(vdev->tph_st_entries, sizeof(u16),
> +					      GFP_KERNEL);

[Severity: Low]
Does this code unconditionally allocate memory for the shadow table even when 
the enable_unsafe_tph module parameter is false? 

While the allocation is relatively small, operations will be rejected with 
-EOPNOTSUPP anyway if the feature is disabled globally. Perhaps this 
allocation can be avoided entirely in that case to save kernel memory.

> +		if (!vdev->tph_st_shadow)
> +			return -ENOMEM;
> +	}

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