Re: [PATCH v8 2/7] PCI/TPH: Export pcie_tph_get_st_modes() for external use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pcie_tph_get_st_modes() and pcie_tph_get_st_table_loc() do not check the return value of pci_read_config_dword(), causing them to return false capabilities on read failures.
--

commit 521881ea093ae852c8fad775f8ffcbe3b9c7a46b
Author: Chengwen Feng <fengchengwen@huawei.com>

PCI/TPH: Export pcie_tph_get_st_modes() for external use

This commit exports the pcie_tph_get_st_modes() helper to allow external
drivers like VFIO to query supported PCIe TPH steering tag modes. It also
adds stub functions for when CONFIG_PCIE_TPH is disabled and includes
tph_cap validation to prevent invalid PCI configuration space access.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 877cf556242b4..ba31b010f67ad 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
> @@ -145,15 +145,27 @@ static void set_ctrl_reg_req_en(struct pci_dev *pdev, u8 req_type)
[ ... ]
> -static u8 get_st_modes(struct pci_dev *pdev)
> +/**
> + * pcie_tph_get_st_modes - Get supported Steering Tag modes
> + * @pdev: PCI device to query
> + *
> + * Return:
> + *  Bitmask of supported ST modes (PCI_TPH_CAP_ST_NS, PCI_TPH_CAP_ST_IV,
> + *                                 PCI_TPH_CAP_ST_DS)
> + */
> +u8 pcie_tph_get_st_modes(struct pci_dev *pdev)
>  {
>  	u32 reg;
>  
> +	if (!pdev->tph_cap)
> +		return 0;
> +
>  	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CAP, &reg);
>  	reg &= PCI_TPH_CAP_ST_NS | PCI_TPH_CAP_ST_IV | PCI_TPH_CAP_ST_DS;
>  
>  	return reg;
>  }

If pci_read_config_dword() fails (for example, if the device is disconnected
or isolated), it sets the output variable reg to PCI_ERROR_RESPONSE (~0U).

Because the return value is not checked, this code will apply the bitmask
to ~0U, which evaluates to 0x07 (PCI_TPH_CAP_ST_NS | PCI_TPH_CAP_ST_IV |
PCI_TPH_CAP_ST_DS).

Since this function is being exported for external use by drivers like VFIO,
could this falsely advertise that the device supports all three ST modes
instead of none? Should the return value of pci_read_config_dword() be
checked to ensure 0 is returned on failure?

> +EXPORT_SYMBOL(pcie_tph_get_st_modes);
>  
>  /**
>   * pcie_tph_get_st_table_loc - Return the device's ST table location
> @@ -168,6 +180,9 @@ u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev)
>  {
>  	u32 reg;
>  
> +	if (!pdev->tph_cap)
> +		return PCI_TPH_LOC_NONE;
> +
>  	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CAP, &reg);
>  
>  	return reg & PCI_TPH_CAP_LOC_MASK;

Similarly, does an unchecked read failure here lead to returning an invalid
location mask derived from the ~0U error response? Would it be safer to check
the return value and return PCI_TPH_LOC_NONE if the read fails?

[ ... ]

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