Re: [PATCH 13/13] PCI: brcmstb: Split up complicated function into two variants

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] An off-by-one error in the inbound window limit check allows one more window than supported by hardware to be processed, leading to out-of-bounds register writes.
- [High] The MSI target address calculation incorrectly hardcodes the array index `inbound_wins[2]`, evaluating either a disabled zero-sized window or uninitialized stack memory instead of the main inbound window.
--

commit 2fa82ffee79e487108d802c77822d346d0a063c9
Author: Jim Quinlan <james.quinlan@broadcom.com>

PCI: brcmstb: Split up complicated function into two variants

This patch refactors brcm_pcie_get_inbound_wins into two variants: one for
newer chips and one for older chips. It updates the configuration data
structures to assign the appropriate function pointer to get_ib_wins.

> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 53984cfa2c96f..45e8a9e030d4a 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
[ ... ]
> @@ -954,8 +957,58 @@ static void add_inbound_win(struct inbound_win *b, u8 *count, u64 size,
>  	(*count)++;
>  }
>  
> +/*
> + * This is used by newer SoCs. It configures the inbound mapping windows
> + * in accordance to the values of the dma-ranges properties.
> + */
>  static int brcm_pcie_get_ib_wins(struct brcm_pcie *pcie,
>  				 struct inbound_win *ib_win)
> +{
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> +	struct resource_entry *entry;
> +	u64 size;
> +	u8 n = 0;
> +
> +	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> +		u64 pcie_start = entry->res->start - entry->offset;
> +		u64 cpu_start = entry->res->start;
> +
> +		size = resource_size(entry->res);
> +		add_inbound_win(ib_win++, &n, size, cpu_start, pcie_start);
> +		if (n > pcie->cfg->num_inbound_wins)
> +			break;

[Severity: High]
This is a pre-existing issue, but does this off-by-one check allow n to
exceed the maximum hardware window count?

Because add_inbound_win() post-increments n, if a device tree provides more
dma-ranges than the maximum supported hardware windows, n can exceed
num_inbound_wins before the break occurs. The function will then return
this oversized window count.

If this happens, when brcm_pcie_set_ib_win_registers() subsequently iterates
over the returned count, could it fetch an out-of-bounds register offset
and overwrite unrelated hardware configuration space?

[ ... ]
> @@ -1195,7 +1217,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK);
>  	writel(tmp, base + PCIE_MISC_MISC_CTRL);
>  
> -	num_inbound_wins = brcm_pcie_get_ib_wins(pcie, inbound_wins);
> +	num_inbound_wins = pcie->cfg->get_ib_wins(pcie, inbound_wins);
>  	if (num_inbound_wins < 0)
>  		return num_inbound_wins;

[Severity: High]
This is a pre-existing issue, but does the setup logic later in this
function rely on inbound_wins[2] being initialized and valid?

In brcm_pcie_setup(), the inbound_wins array is allocated on the stack.
Later in the function, it calculates the MSI target address like this:

    if (inbound_wins[2].pci_offset >= SZ_4G ||
        (inbound_wins[2].size + inbound_wins[2].pci_offset) < SZ_4G)
            pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
    else
            pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;

For newer SoCs using brcm_pcie_get_ib_wins(), if the device tree provides
fewer than 3 dma-ranges entries, could this logic read uninitialized stack
memory?

For older SoCs using brcm_pcie_get_ib_wins_internal_map(), inbound_wins[2]
is deliberately populated as a zero-sized, disabled window. Does evaluating
this zeroed window result in misconfiguring the MSI target address,
potentially causing PCIe device interrupts to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911233541.1650895-1-james.quinlan@broadcom.com?part=13