Re: [PATCH v19 11/18] vfio/pci: Virtualize PCIe TPH capability registers

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Out-of-bounds read and write on the `perm->virt` and `perm->write` kernel heap buffers due to unvalidated dynamic hardware TPH capability length.

Pre-existing issues:
- [Critical] Missing bounds check in `vfio_ecap_init` can lead to a heap buffer overflow when handling extended capabilities with dynamically parsed lengths.
--

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

vfio/pci: Virtualize PCIe TPH capability registers

This patch implements virtualization and policy masking for the PCIe TPH
extended capability. It splits configuration space permissions to keep the
header read-only while marking TPH_CTRL and ST-table entries as virtually
writable, and adjusts virtual capability bits based on hardware support.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 5c6ab172df6c0..06d7b2fbf866a 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1086,6 +1086,73 @@ 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]))

[Severity: Critical]
Does this code under-allocate the permission bits if the capability length
is dynamically larger than 256 bytes?

Because pci_ext_cap_length[PCI_EXT_CAP_ID_TPH] is 0xFF, alloc_perm_bits()
will allocate a fixed 256-byte buffer. If vfio_ext_cap_len() calculates a
dynamically larger TPH capability size based on a device's ST table, it
appears subsequent user space reads or writes past offset 255 could cause an
out-of-bounds heap access when vfio_default_config_read() or
vfio_default_config_write() calls memcpy() against perm->virt and perm->write.

> +		return -ENOMEM;
> +
> +	p_setd(perm, 0, ALL_VIRT, NO_WRITE);

[ ... ]

> @@ -1704,6 +1774,8 @@ static int vfio_ecap_init(struct vfio_pci_core_device *vdev)
>  		ret = vfio_fill_vconfig_bytes(vdev, epos, len);
>  		if (ret)
>  			return ret;
> +		if (ecap == PCI_EXT_CAP_ID_TPH && !hidden)
> +			vfio_tph_capability_adjust(vdev, epos);

[Severity: Critical]
This is a pre-existing issue, but could the dynamically calculated length lead
to a heap buffer overflow here?

If a faulty or malicious device advertises a large ST table size,
vfio_ext_cap_len() can calculate a length where epos + len significantly
exceeds pdev->cfg_size. Since there appears to be no bounds check before:

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

could this overflow the vdev->pci_config_map and vdev->vconfig heap buffers?

>  
>  		/*
>  		 * If we're just using this capability to anchor the list,

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