Re: [PATCH v16 08/12] vfio/pci: Hide TPH capability when TPH is unsupported

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds check on parsed PCIe capability length causes heap buffer overflow.
--

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

vfio/pci: Hide TPH capability when TPH is unsupported

Check the device negotiated TPH support status before parsing the TPH
extended capability. Return zero length to hide the capability from
userspace if TPH is disabled during topology negotiation.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a10ed733f0e3a..5c6ab172df6c0 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
[ ... ]
> @@ -1450,6 +1451,8 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
>  		byte &= PCI_DPA_CAP_SUBSTATE_MASK;
>  		return PCI_DPA_BASE_SIZEOF + byte + 1;
>  	case PCI_EXT_CAP_ID_TPH:
> +		if (!pcie_tph_supported(pdev, false))
> +			return 0;
>  		ret = pci_read_config_dword(pdev, epos + PCI_TPH_CAP, &dword);
>  		if (ret)
>  			return pcibios_err_to_errno(ret);

[Severity: Critical]
This is a pre-existing issue, but since we are adding support for another
variable-length capability, can the capability length returned here cause a
heap buffer overflow?

Looking at vfio_ecap_init() in drivers/vfio/pci/vfio_pci_config.c, the
returned length is used in memset() and vfio_fill_vconfig_bytes() without
bounds checking against the size of the config space buffer:

drivers/vfio/pci/vfio_pci_config.c:vfio_ecap_init() {
    ...
    len = vfio_ext_cap_len(vdev, ecap, epos);
    ...
    memset(map + epos, ecap, len);
    ret = vfio_fill_vconfig_bytes(vdev, epos, len);
    ...
}

If a malicious or compromised PCIe endpoint provides forged values for
capabilities like TPH or VNDR, resulting in a length that makes
epos + len > pdev->cfg_size, would this result in an out-of-bounds write
past the boundaries of the map and vconfig heap buffers?

A similar issue appears to exist for standard capabilities in vfio_cap_init()
where length is also not bounds-checked:

drivers/vfio/pci/vfio_pci_config.c:vfio_cap_init() {
    ...
    memset(map + pos, cap, len);
    ret = vfio_fill_vconfig_bytes(vdev, pos, len);
    ...
}

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