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

[fengchengwen <fengchengwen@huawei.com>]

Hi Alex,

On 7/2/2026 9:04 PM, sashiko-bot@kernel.org wrote:
> 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.

Raw vfio_pci_set_power_state() does not restore revoked dma-buf mappings, which
theoretically may trigger IOMMU faults for guests. There is a local helper
vfio_lock_and_set_power_state() that takes memory locks and runs
vfio_pci_dma_buf_move(vdev, false) after switching to D0 to rebuild DMA
IOVA mappings.

I’m considering replacing direct vfio_pci_set_power_state(PCI_D0) calls
in TPH write and TPH_ST paths with this wrapper. Could you help confirm
whether this is a valid concern and if this wrapper usage is acceptable here,
or if there are any hidden edge cases I need to handle?


> - [Medium] The `VFIO_DEVICE_FEATURE_SET` operation for TPH fails to copy and validate the userspace payload, breaking forward compatibility.

Will add copy_from_user + zero check in next version.

> 
> 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.

Pre-existing generic issue, separate cleanup patch later.

Thanks

> --
> 
> 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?
>