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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `pcie_tph_get_st_modes()` and `pcie_tph_get_st_table_loc()` functions ignore the return value of `pci_read_config_dword()`, leading to fabricated capabilities being exposed to userspace.
- [Low] The commit message incorrectly refers to the configuration option as `!CONFIG_PCI_TPH` instead of `!CONFIG_PCIE_TPH`.
--

commit 4a10965994892856e6b789d0c75e23d0e2e09be7
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 so drivers like VFIO
can query and expose device capabilities to userspace. It also adds stub
functions and tph_cap validation to prevent invalid configuration access.

> Add stub functions for pcie_tph_get_st_table_size() and
> pcie_tph_get_st_table_loc() when !CONFIG_PCI_TPH.

This isn't a bug, but there is a typo in the configuration option name.
The actual kernel configuration option for PCIe TLP Processing Hints is
CONFIG_PCIE_TPH rather than CONFIG_PCI_TPH.

> 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
[ ... ]
> -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);

Should we check the return value of pci_read_config_dword() here?

If the device is disconnected or unresponsive, pci_read_config_dword()
returns an error and populates the variable with the standard PCI error
response 0xFFFFFFFF.

When the mask (PCI_TPH_CAP_ST_NS | PCI_TPH_CAP_ST_IV | PCI_TPH_CAP_ST_DS)
is applied to 0xFFFFFFFF, it results in 7. This falsely indicates that all
Steering Tag modes are supported.

Since this function is now exported for external subsystems like VFIO, does
this risk exposing fabricated hardware capabilities to userspace when a
device becomes unresponsive?

>  	reg &= PCI_TPH_CAP_ST_NS | PCI_TPH_CAP_ST_IV | PCI_TPH_CAP_ST_DS;
>  
>  	return reg;
>  }
> +EXPORT_SYMBOL(pcie_tph_get_st_modes);
[ ... ]
>  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);

Similarly, does this need error checking for the config read?

Applying the PCI_TPH_CAP_LOC_MASK (3 << 9) to 0xFFFFFFFF results in 0x600,
which is an invalid, undefined location enum.

>  
>  	return reg & PCI_TPH_CAP_LOC_MASK;
>  }

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