Re: [PATCH v17 01/12] PCI/TPH: Fix pcie_tph_get_st_table_loc() field extraction

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Surprise removal (PCI disconnect) or a malicious PCI endpoint can trigger a heap buffer overflow in `pci_save_tph_state()` and an OOB read in `pci_restore_tph_state()` due to a TOCTOU evaluation of the ST table size.
--

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

PCI/TPH: Fix pcie_tph_get_st_table_loc() field extraction

pcie_tph_get_st_table_loc() incorrectly uses FIELD_GET(), which shifts the
field value to bit 0. But the function is designed to return raw
PCI_TPH_LOC_* values as defined in the function comment.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 91145e8d9d951..bef3a55539c46 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
[ ... ]
> @@ -185,9 +188,6 @@ u16 pcie_tph_get_st_table_size(struct pci_dev *pdev)
>  
>  	/* Check ST table location first */
>  	loc = pcie_tph_get_st_table_loc(pdev);
> -
> -	/* Convert loc to match with PCI_TPH_LOC_* defined in pci_regs.h */
> -	loc = FIELD_PREP(PCI_TPH_CAP_LOC_MASK, loc);
>  	if (loc != PCI_TPH_LOC_CAP)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does this function have a TOCTOU race
that could cause a buffer overflow during a device disconnect?

pcie_tph_get_st_table_loc() indirectly reads the PCI_TPH_CAP register.
Shortly after, pcie_tph_get_st_table_size() reads it again:

drivers/pci/tph.c:pcie_tph_get_st_table_size() {
    ...
	loc = pcie_tph_get_st_table_loc(pdev);
	if (loc != PCI_TPH_LOC_CAP)
		return 0;

	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CAP, &reg);

	return FIELD_GET(PCI_TPH_CAP_ST_MASK, reg) + 1;
}

If a device disconnects exactly between these two reads, or if a malicious
endpoint provides PCI_TPH_LOC_CAP for the first read but ~0 for the second
read, the FIELD_GET() calculation evaluates to 2048.

Since pci_save_tph_state() iterates based on this dynamically queried size,
could it write up to 4096 bytes of disconnect error data into the
save_state->cap.data buffer, overflowing it?

drivers/pci/tph.c:pci_save_tph_state() {
    ...
	num_entries = pcie_tph_get_st_table_size(pdev);
	for (i = 0; i < num_entries; i++) {
		pci_read_config_word(pdev, pdev->tph_cap + offset,
				     st_entry++);
    ...
}

Similarly, could pci_restore_tph_state() be vulnerable to an out-of-bounds
heap read if the table size evaluates to 2048 on disconnect?

drivers/pci/tph.c:pci_restore_tph_state() {
    ...
	num_entries = pcie_tph_get_st_table_size(pdev);
	for (i = 0; i < num_entries; i++) {
		pci_write_config_word(pdev, pdev->tph_cap + offset,
				      *st_entry++);
    ...
}

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