Re: [PATCH v16 12/12] vfio/pci: Virtualize PCIe TPH capability registers

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Concurrent config writes cause Double-Free/Use-After-Free in power management saved state.
- [High] Missing virtual configuration rollback on power state transition failure.
- [High] Missing virtualization of TPH Capability Header leaks the physical Next Capability Pointer to the guest.
- [High] Race condition (TOCTOU) allows concurrent config writes to corrupt virtual state and silently lose write requests.
- [Medium] Silent failure when programming TPH shadow ST table entries into hardware.

Pre-existing issues:
- [High] Out-of-bounds write in extended capability parsing loop due to missing boundary checks.
--

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

vfio/pci: Virtualize PCIe TPH capability registers

This commit virtualizes TPH extended capability registers, splitting
permissions so the header is read-only and the control register is writable.
It restricts ST-table programming to a specific feature and syncs the
cached shadow table to hardware upon enablement.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 5c6ab172df6c0..23c32aa5842fa 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1086,6 +1086,109 @@ static int __init init_pci_ext_cap_pwr_perm(struct perm_bits *perm)
>  	return 0;
>  }
>  
> +/* Permissions for TPH extended capability */
> +static int __init init_pci_ext_cap_tph_perm(struct perm_bits *perm)
> +{
> +	int i;
> +
> +	if (alloc_perm_bits(perm, pci_ext_cap_length[PCI_EXT_CAP_ID_TPH]))
> +		return -ENOMEM;
> +
> +	p_setd(perm, PCI_TPH_CAP, ALL_VIRT, NO_WRITE);

[Severity: High]
Does this miss virtualizing the TPH Capability Header at offset 0?

Other capabilities use p_setd(perm, 0, ALL_VIRT, NO_WRITE) to ensure the
guest reads the virtualized header instead of the physical one. Without this,
could guest reads to the TPH capability header fall through to physical
hardware, leaking the physical Next Capability Pointer and breaking VFIO's
ability to hide capabilities?

[ ... ]

> +static int vfio_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
> +				 int count, struct perm_bits *perm,
> +				 int offset, __le32 val)
> +{
> +	int req_en_byte = PCI_TPH_CTRL + 1;
> +	struct pci_dev *pdev = vdev->pdev;
> +	__le32 org_val = 0;
> +	bool extended;
> +	u8 mode, req;
> +	int i, ret;
> +	u16 start;
> +	u32 data;
> +
> +	if (!vdev->tph_permit)
> +		return count;
> +
> +	/* Back up the original values in order rollback when fail */
> +	if (offset <= req_en_byte && offset + count > req_en_byte)
> +		vfio_default_config_read(vdev, pos, count, perm, offset,
> +					 &org_val);
> +
> +	ret = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	if (ret != count)
> +		return ret;

[Severity: High]
Can this create a race condition where concurrent writes corrupt the virtual
state?

Modifying vconfig via vfio_default_config_write() happens before acquiring
vdev->tph_lock. If two threads concurrently write to the control register,
both modify vconfig unprotected. If the first thread acquires the lock but
fails to program hardware, its rollback would blindly overwrite the second
thread's new state in vconfig.

When the second thread subsequently acquires the lock, wouldn't it read the
reverted vconfig value (the first thread's old state) and write it to
hardware, effectively discarding the second thread's write while returning
success?

> +
> +	/* Skip if write range does not cover Requester Enable byte */
> +	if (offset > req_en_byte || offset + count <= req_en_byte)
> +		return count;
> +
> +	ret = vfio_pci_set_power_state(vdev, PCI_D0);

[Severity: Critical]
Does calling vfio_pci_set_power_state() without a serializing lock expose a
double-free vulnerability?

If a guest VM concurrently writes to the TPH control register while the
physical device is in D3hot, both threads can evaluate needs_restore = true.
After the first thread transitions the device to D0, both threads will
concurrently execute pci_load_and_free_saved_state(pdev, &vdev->pm_save).

Since pci_load_and_free_saved_state() has no internal locking and directly
frees the shared pointer, could this concurrent execution cause a double-free
in the host kernel?

> +	if (ret)
> +		return count;

[Severity: High]
Is a rollback of the virtual configuration missing in this error path?

If the power state transition fails, the function returns count (indicating
success to userspace) but fails to roll back vconfig using the org_val backed
up earlier. Doesn't this leave vconfig falsely indicating that the TPH
Requester is configured while the physical hardware remains unprogrammed?

> +
> +	mutex_lock(&vdev->tph_lock);
> +
> +	start = vfio_find_cap_start(vdev, pos);
> +	data = le32_to_cpu(*(__le32 *)&vdev->vconfig[start + PCI_TPH_CTRL]);
> +	mode = FIELD_GET(PCI_TPH_CTRL_MODE_SEL_MASK, data);
> +	req = FIELD_GET(PCI_TPH_CTRL_REQ_EN_MASK, data);
> +
> +	if (req == PCI_TPH_REQ_TPH_ONLY || req == PCI_TPH_REQ_EXT_TPH) {
> +		extended = !!(req == PCI_TPH_REQ_EXT_TPH);
> +		ret = pcie_enable_tph_explicit(pdev, mode, extended);
> +		if (!ret && vdev->tph_st_shadow) {
> +			for (i = 0; i < vdev->tph_st_entries; i++)
> +				pcie_tph_set_st_entry(pdev, i,
> +						      vdev->tph_st_shadow[i]);

[Severity: Medium]
Should the return value of pcie_tph_set_st_entry() be checked here?

If programming any ST entry fails, such as from hardware rejection or
inaccessible registers, the failure appears to be silently ignored. Could
this leave the hardware partially programmed while userspace is told the
operation succeeded?

> +		}

[ ... ]

> @@ -1704,6 +1810,8 @@ static int vfio_ecap_init(struct vfio_pci_core_device *vdev)
>  		ret = vfio_fill_vconfig_bytes(vdev, epos, len);

[Severity: High]
This is a pre-existing issue, but could this capability parsing loop allow
out-of-bounds heap corruption?

If a malicious hardware device or hotplugged peripheral provides a malformed
capability list with a large length near the end of the PCIe config space,
epos + len can exceed pdev->cfg_size. This would cause a buffer overflow in
memset() and vfio_fill_vconfig_bytes(), corrupting the heap-allocated
vdev->pci_config_map and vdev->vconfig buffers.

>  		if (ret)
>  			return ret;
> +		if (ecap == PCI_EXT_CAP_ID_TPH && !hidden)
> +			vfio_tph_mask_ext_tph_bit(vdev, epos);

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