Re: [PATCH v9 3/5] PCI: tegra: Add Tegra264 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver incorrectly accesses the ECAM window via `pcie->cfg->win` to read the root port configuration space, causing an out-of-bounds heap read on 32-bit platforms.
- [High] The driver incorrectly uses the Negotiated Link Width (NLW) to set interconnect bandwidth for hotplug-capable ports when the link is down, permanently setting it to 0.
--

--- Patch [3]: [PATCH v9 3/5] PCI: tegra: Add Tegra264 support ---
commit af63006f768e2991e49a48de9855ea58d20662ed
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.

> diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c
> new file mode 100644
> index 0000000000000..e0c5b9d0d20bf
> --- /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]));

[Severity: High]
Will this permanently set the interconnect bandwidth to 0 for hotplug-capable
ports if the link is initially down?

When the link is down, the Negotiated Link Width (NLW) is 0, which means
the bandwidth value will be calculated as 0. Since the generic pciehp driver
handles hotplug but doesn't know about this driver's specific interconnect
bandwidth requirements, it appears there is no way to dynamically increase
the bandwidth later when a device is actually inserted.

Should this read the Maximum Link Width and Speed from PCI_EXP_LNKCAP
instead of PCI_EXP_LNKSTA to properly reserve bandwidth for a hotplug slot?

> +
> +	/*
> +	 * 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);
[ ... ]
> +static int tegra264_pcie_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	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 cause an out-of-bounds heap read on 32-bit platforms?

In pci_ecam_create(), per_bus_mapping is enabled by default on 32-bit builds
(!IS_ENABLED(CONFIG_64BIT)). When per_bus_mapping is active, the win member
of struct pci_config_window aliases to winp (an array of pointers) because
they are defined in a union.

By assigning pcie->ecam = pcie->cfg->win here, the driver stores a pointer
to this heap array. Later, when the driver attempts to access the root port
configuration space:

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

will this read arbitrary heap memory as if it were MMIO?

> +
> +	tegra264_pcie_init(pcie);
> +
> +	/*
> +	 * Fail if the link isn't up and doesn't support hotplug, no device
> +	 * will ever be able to be added on this bus.
> +	 */
> +	if (!pcie->link_up && !pcie->supports_hotplug) {
> +		err = dev_err_probe(pcie->dev, -ENODEV,
> +				    "PCIe #%u link is down and not hotplug-capable, turning off\n",
> +				    pcie->ctl_id);
> +		tegra264_pcie_power_off(pcie);
> +		goto err_free_ecam;
> +	}
> +
> +	err = pci_host_probe(bridge);
> +	if (err < 0) {
> +		dev_err_probe(dev, err, "failed to register host\n");
> +		goto err_free_ecam;
> +	}
> +
> +	return 0;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-tegra264-pcie-v9-0-fa2ed7350ae1@nvidia.com?part=3