Re: [PATCH v19 14/18] vfio/pci: Implement VFIO_DEVICE_FEATURE_TPH and valid TPH config write support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Waking the device to PCI_D0 during TPH configuration writes or ST table programming fails to restore zapped DMA buffers, permanently breaking device DMA.
- [Medium] The `VFIO_DEVICE_FEATURE_SET` operation for TPH fails to copy and validate the userspace payload, breaking forward compatibility.

Pre-existing issues:
- [High] Missing bounds checking in PCI capability parsing loops allows a malicious or spoofed PCIe endpoint to overflow the `vconfig` and `pci_config_map` heap buffers.
--

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

vfio/pci: Implement VFIO_DEVICE_FEATURE_TPH and valid TPH config write support

This commit adds support for VFIO_DEVICE_FEATURE_TPH in
vfio_pci_core_feature_tph, allowing GET and SET operations for TPH features.
It also implements vfio_tph_config_write to support TPH_CTRL modifications once
TPH is permitted, managing power states as required for writing TPH registers.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 06d7b2fbf866a..388dd6fed16b5 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1150,6 +1150,60 @@ static int vfio_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
>  				 int count, struct perm_bits *perm,
>  				 int offset, __le32 val)
>  {
[ ... ]
> +	org_req = FIELD_GET(PCI_TPH_CTRL_REQ_EN_MASK, org_ctrl);
> +	if (req == org_req)
> +		goto out; /* Only care about requester enable */
> +
> +	ret = vfio_pci_set_power_state(vdev, PCI_D0);
> +	if (ret)
> +		goto restore; /* Drop this write */

[Severity: High]
Does waking the device to PCI_D0 here miss the required vfio_pci_dma_buf_move()
call to restore DMA mappings?

If the device is in a low-power state (like D3hot) and its DMA buffers are
zapped, forcing it to D0 without restoring the DMA mappings could leave the
device active but unable to perform DMA, potentially causing IOMMU faults and
breaking device functionality. Does the same issue also occur in the ioctl path
for ST table batch programming?

[ ... ]
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f11a9624b9c0..14944d3ea86e6 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1607,6 +1609,38 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_core_feature_tph(struct vfio_pci_core_device *vdev,
> +				     u32 flags,
> +				     struct vfio_device_feature_tph __user *arg,
> +				     size_t argsz)
> +{
> +	struct vfio_device_feature_tph tph = {0};
> +	int ret;
> +
> +	if (!pcie_tph_supported(vdev->pdev, false))
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz,
> +			VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET,
> +			sizeof(tph));
> +	if (ret <= 0)
> +		return ret;
> +
> +	if (flags & VFIO_DEVICE_FEATURE_SET) {
> +		vdev->tph_permit = 1;
> +		return 0;
> +	}

[Severity: Medium]
Does this code silently ignore userspace payloads for the VFIO_DEVICE_FEATURE_SET
operation by skipping copy and validation?

Future kernel extensions that add new flags to struct vfio_device_feature_tph
for the SET operation could be silently ignored by older kernels, which might
break userspace's ability to safely probe or use new features. Should it verify
that the payload is zeroed?

[ ... ]

While reviewing this code, I also noticed a pre-existing issue in
vfio_ecap_init() in drivers/vfio/pci/vfio_pci_config.c:

[Severity: High]
This is a pre-existing issue, but does the while loop in vfio_ecap_init()
lack a bounds check for the heap buffers?

drivers/vfio/pci/vfio_pci_config.c:vfio_ecap_init() {
[ ... ]
	while (loops-- && epos >= PCI_CFG_SPACE_SIZE) {
[ ... ]
		for (i = 0; i < len; i++) {
			if (likely(map[epos + i] == PCI_CAP_ID_INVALID))
				continue;
[ ... ]
		memset(map + epos, ecap, len);
		ret = vfio_fill_vconfig_bytes(vdev, epos, len);
[ ... ]
}

The loop condition only checks that epos >= PCI_CFG_SPACE_SIZE, but never
verifies that epos + len <= pdev->cfg_size before accessing the heap
allocations (map and vconfig). Could a malicious or spoofed PCIe endpoint
provide an ecap with an epos and len that overflows these buffers?

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