Re: [PATCH v8 4/6] PCI: tegra: Add Tegra264 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Heap out-of-bounds read on 32-bit platforms due to union aliasing of `pci_config_window->win`.
- [Medium] Missing runtime PM usage counter decrement on `pm_runtime_get_sync()` failure.
- [Low] Incorrect unit multiplier used in bandwidth request error message.
--

--- Patch [4]: [PATCH v8 4/6] PCI: tegra: Add Tegra264 support ---
commit 4da3b5a2d216f984d21597a5d585e8a8eb97b293
Author: Thierry Reding <treding@nvidia.com>

PCI: tegra: Add Tegra264 support
    
Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The
driver is very small, with its main purpose being to set up the address
translation registers and then creating a standard PCI host using ECAM.

> --- /dev/null
> +++ b/drivers/pci/controller/pcie-tegra264.c

[ ... ]

> +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)
> +{
> +	u32 value, speed, width;
> +	int err;
> +
> +	value = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);
> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);
> +
> +	value = Mbps_to_icc(width * PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]));
> +
> +	/*
> +	 * We don't want to error out here because a boot-critical device
> +	 * could be connected to this root port. Failure to set the bandwidth
> +	 * request may have an adverse impact on performance, but it is not
> +	 * generally fatal, so we opt to continue regardless so that users
> +	 * get a chance to fix things.
> +	 */
> +	err = icc_set_bw(pcie->icc_path, value, value);
> +	if (err < 0)
> +		dev_err(pcie->dev,
> +			"failed to request bandwidth (%u MBps): %pe\n",
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
This isn't a bug, but does this log message print the wrong units?

The value calculated by Mbps_to_icc() appears to be in kilobytes per
second, but the message labels it as "MBps".

[ ... ]

> +static int tegra264_pcie_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	pcie->bpmp = tegra_bpmp_get_with_id(dev, &pcie->ctl_id);
> +	if (IS_ERR(pcie->bpmp))
> +		return dev_err_probe(dev, PTR_ERR(pcie->bpmp),
> +				     "failed to get BPMP\n");
> +
> +	err = devm_pm_runtime_set_active_enabled(dev);
> +	if (err < 0) {
> +		dev_err_probe(dev, err, "failed to enable runtime PM\n");
> +		goto err_put_bpmp;
> +	}
> +
> +	err = pm_runtime_get_sync(dev);
> +	if (err < 0) {
> +		dev_err_probe(dev, err, "failed to power on device\n");
> +		goto err_put_bpmp;
                ^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Does this error path leak the runtime PM usage counter?

When pm_runtime_get_sync() fails, it still increments the usage counter. By
jumping directly to err_put_bpmp, the counter is not decremented, which
could prevent the device from entering runtime suspend if probed again later.

> +	}
> +
> +	/* sanity check that programmed ranges match what's in DT */
> +	if (!tegra264_pcie_check_ranges(pdev)) {
> +		err = -EINVAL;
> +		goto err_put_pm;
> +	}
> +
> +	pcie->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);
> +	if (IS_ERR(pcie->cfg)) {
> +		err = dev_err_probe(dev, PTR_ERR(pcie->cfg),
> +				    "failed to create ECAM\n");
> +		goto err_put_pm;
> +	}
> +
> +	bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
> +	bridge->sysdata = pcie->cfg;
> +	pcie->ecam = pcie->cfg->win;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can this assignment cause an out-of-bounds heap read on 32-bit platforms?

If the driver is compiled on a 32-bit architecture (e.g. via COMPILE_TEST)
where per_bus_mapping is true, the pci_config_window->win union aliases the
dynamically allocated cfg->winp array pointer.

When the driver later dereferences pcie->ecam using readw() in
tegra264_pcie_link_up():

    u16 value = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);

It will read from the heap array pointer instead of mapped I/O memory,
resulting in a crash or garbage read.

> +
> +	tegra264_pcie_init(pcie);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-tegra264-pcie-v8-0-23e51589229b@nvidia.com?part=4