Re: [PATCH] PCI: cadence-plat: Fix host/endpoint dependencies

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

Pre-existing issues:
- [High] Invalid drvdata cast in cdns_plat_pcie_shutdown causes out-of-bounds access and kernel panic.
- [High] Probe error paths unconditionally return 0, masquerading initialization failures as success.
--

--- Patch [1]: [PATCH] PCI: cadence-plat: Fix host/endpoint dependencies ---
commit f31c2dc6bdd9c13d43549cb7b505f8e0289ac1c5
Author: Aksh Garg <[email protected]>

PCI: cadence-plat: Fix host/endpoint dependencies

The cadence-plat driver has a single platform driver that can be built-in
or a loadable module, but it calls two separate backend drivers depending
on whether it is a host or endpoint. Fix this by moving the 'select' of 
PCIE_CADENCE_HOST and PCIE_CADENCE_EP into the common PCIE_CADENCE_PLAT 
symbol.

Link: https://lore.kernel.org/linux-next/[email protected]/

> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> index 9e651d5459735..fd704dec47dda 100644
> --- a/drivers/pci/controller/cadence/Kconfig
> +++ b/drivers/pci/controller/cadence/Kconfig

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
drivers/pci/controller/cadence/pcie-cadence-plat.c:cdns_plat_pcie_shutdown().
The function casts the drvdata to struct cdns_pcie:

drivers/pci/controller/cadence/pcie-cadence-plat.c:cdns_plat_pcie_shutdown() {
	struct cdns_pcie *pcie = dev_get_drvdata(dev);
	...
	cdns_pcie_disable_phy(pcie);
}

However, the probe function allocates a smaller wrapper struct and sets it
as drvdata:

drivers/pci/controller/cadence/pcie-cadence-plat.c:cdns_plat_pcie_probe() {
	cdns_plat_pcie = devm_kzalloc(dev, sizeof(*cdns_plat_pcie), GFP_KERNEL);
	...
	platform_set_drvdata(pdev, cdns_plat_pcie);
}

Since cdns_pcie_disable_phy() attempts to read pcie->phy_count and iterates
over pcie->phy[i], could this out-of-bounds access cause a kernel panic
during shutdown?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a problem
with the error handling in the probe function:

drivers/pci/controller/cadence/pcie-cadence-plat.c:cdns_plat_pcie_probe() {
	...
 err_init:
 err_get_sync:
	pm_runtime_put_sync(dev);
	pm_runtime_disable(dev);
	cdns_pcie_disable_phy(cdns_plat_pcie->pcie);
	phy_count = cdns_plat_pcie->pcie->phy_count;
	while (phy_count--)
		device_link_del(cdns_plat_pcie->pcie->link[phy_count]);

	return 0;
}

Does this unconditionally return 0 on failure paths?

It looks like the error code is dropped, which would masquerade probe
failures as successes. This could leave the driver bound while its resources
are actually disabled, potentially leading to crashes on unbind or shutdown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1