Re: [PATCH v14 6/8] vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST_CONFIG for PCIe TPH ST configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The ioctl handler accesses device hardware registers without ensuring the device is in the D0 power state, which can cause host crashes.
- [Medium] The shadow ST table is not populated with the initial hardware state, leading to hardware state corruption upon rollback.
- [Low] Mixing goto-based error handling with scope-based cleanup (`guard()`) violates kernel cleanup guidelines.
- [Low] Shadow ST table memory is allocated unconditionally during device initialization, ignoring the module parameter gate.
--

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

vfio/pci: Add VFIO_DEVICE_FEATURE_TPH_ST_CONFIG for PCIe TPH ST configuration
    
This commit adds a new VFIO device feature to allow userspace to configure
PCIe TPH Steering Tag table entries. It implements a shadow ST table to cache
entries and introduces a module parameter to gate the feature.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 050e7542952ed..c9035c0acaf08 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -1551,6 +1563,116 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_tph_st_shadow_size(struct vfio_pci_core_device *vdev)

[ ... ]

> +static int vfio_pci_tph_init(struct vfio_pci_core_device *vdev)
> +{
> +	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]
Since this feature is gated by the enable_unsafe_tph module parameter during
ioctl execution, should this memory allocation also be gated by that parameter?

If the feature is disabled (the default), this allocates memory for every
TPH-capable device that will never be used.

> +		if (!vdev->tph_st_shadow)
> +			return -ENOMEM;
> +	}
> +
> +	mutex_init(&vdev->tph_lock);
> +
> +	return 0;
> +}

[ ... ]

> +static int vfio_pci_core_feature_tph_st_config(
> +			struct vfio_pci_core_device *vdev,
> +			u32 flags,
> +			struct vfio_device_feature_tph_st_config __user *arg,
> +			size_t argsz)
> +{
> +	struct vfio_device_feature_tph_st_config config;
> +	struct pci_dev *pdev = vdev->pdev;
> +	void __user *uptr;
> +	int i, idx, ret;
> +	size_t sz;
> +	u16 *sts;
> +
> +	if (!enable_unsafe_tph || !vdev->tph_st_shadow)
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz,
> +				 VFIO_DEVICE_FEATURE_SET |
> +				 VFIO_DEVICE_FEATURE_PROBE,
> +				 sizeof(config));
> +	if (ret <= 0)
> +		return ret;
> +
> +	if (copy_from_user(&config, arg, sizeof(config)))
> +		return -EFAULT;
> +
> +	if (config.count == 0 || config.index >= vdev->tph_st_entries ||
> +		config.count > vdev->tph_st_entries ||
> +		config.index + config.count > vdev->tph_st_entries ||
> +		config.reserved != 0)
> +		return -EINVAL;
> +
> +	guard(mutex)(&vdev->tph_lock);

[Severity: Low]
Is it okay to mix guard() based locking with goto-based error handling in the
same function?

According to include/linux/cleanup.h, the expectation is to convert all
resources to scope-based cleanup or convert none of them. This code uses a
goto rollback pattern and manual kfree() alongside guard().

> +
> +	uptr = u64_to_user_ptr(config.data_uptr);
> +	sts = memdup_array_user(uptr, config.count, sizeof(u16));
> +	sz = config.count * sizeof(u16);
> +	if (IS_ERR(sts))
> +		return PTR_ERR(sts);
> +
> +	if (pcie_tph_enabled_req_type(pdev) == PCI_TPH_REQ_DISABLE) {
> +		memcpy(&vdev->tph_st_shadow[config.index], sts, sz);
> +		kfree(sts);
> +		return 0;
> +	}
> +
> +	for (i = 0; i < config.count; i++) {
> +		idx = config.index + i;
> +		ret = pcie_tph_set_st_entry(pdev, idx, sts[i]);

[Severity: High]
Does this code need to ensure the device is in the D0 power state before
accessing its hardware registers?

The ioctl handler directly calls pcie_tph_set_st_entry() which accesses the
device's PCI configuration space or memory-mapped MSI-X table. If userspace
has placed the device into a low-power state (like D3hot) and no runtime PM
reference is acquired here, accessing MMIO regions could result in Unsupported
Requests and potentially trigger a fatal machine check exception.

> +		if (ret)
> +			goto rollback;
> +	}
> +
> +	memcpy(&vdev->tph_st_shadow[config.index], sts, sz);
> +	kfree(sts);
> +	return 0;
> +
> +rollback:
> +	while (i-- > 0) {
> +		idx = config.index + i;
> +		pcie_tph_set_st_entry(pdev, idx, vdev->tph_st_shadow[idx]);

[Severity: Medium]
Will this rollback write zeros to the hardware during the first ioctl call?

The shadow ST table vdev->tph_st_shadow is allocated with kcalloc and never
synchronized with the initial hardware state. If a batch write fails after
successfully modifying some entries, this rollback loop appears to write the
zeroed shadow contents back to the hardware, potentially overwriting valid
BIOS- or host-configured ST entries instead of their true initial values.

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

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