Re: [PATCH v7 3/3] PCI: cadence: Add LGA IP debugfs for LTSSM status

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Invalid pointer derivation via `to_pci_epc()` in `cdns_pcie_ep_disable()` leading to a wild pointer dereference.
- [High] The `pcie-cadence-plat.c` driver incorrectly returns 0 on probe failure and lacks a `.remove` callback, leading to silent probe failures and severe use-after-free bugs on unbind.
--

commit b49dbfe29c305b407944a72dcdee68320ac08c7c
Author: Hans Zhang <18255117159@163.com>

PCI: cadence: Add LGA IP debugfs for LTSSM status

This commit extends debugfs support to LGA-based Cadence PCIe controllers.
The 'ltssm_status' file now supports both HPA and LGA IP by selecting the
appropriate register access based on the 'is_hpa' flag.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index c0e1194a936b3..370b19f4d38fb 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -655,6 +655,7 @@ void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
>  	struct device *dev = ep->pcie.dev;
>  	struct pci_epc *epc = to_pci_epc(dev);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a wild pointer
dereference?

In cdns_pcie_ep_setup(), the epc object is allocated separately using
devm_pci_epc_create(), which embeds its own struct device (epc->dev).

Here in cdns_pcie_ep_disable(), the code attempts to recover the epc pointer
by passing the parent platform device (ep->pcie.dev) to to_pci_epc().

Because to_pci_epc() is a container_of macro, subtracting the offset of the
dev member from the platform device pointer will yield an invalid pointer.

Will dereferencing this pointer in downstream functions like
pci_epc_deinit_notify() cause memory corruption or a kernel panic during
driver unbind?

>  
> +	cdns_pcie_debugfs_deinit(&ep->pcie);
>  	pci_epc_deinit_notify(epc);
>  	pci_epc_mem_free_addr(epc, ep->irq_phys_addr, ep->irq_cpu_addr,
>  			      SZ_128K);

[ ... ]

I also noticed a pre-existing issue in drivers/pci/controller/cadence/pcie-cadence-plat.c:

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;
}

[Severity: High]
This isn't a bug introduced by this patch, but does returning 0 here silently
mask probe failures from the driver core?

Also, the platform driver lacks a .remove callback:

static struct platform_driver cdns_plat_pcie_driver = {
	.driver = {
		.name = "cdns-pcie",
		.of_match_table = cdns_plat_pcie_of_match,
		.pm	= &cdns_pcie_pm_ops,
	},
	.probe = cdns_plat_pcie_probe,
	.shutdown = cdns_plat_pcie_shutdown,
};

Does the lack of a .remove callback mean no driver-specific teardown runs
on unbind?

Without it, devres will free the controller memory while leaving the active
PCI root bus and debugfs nodes with dangling pointers.  Could this result in
use-after-free bugs when those pointers are accessed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611165933.20071-1-18255117159@163.com?part=3