Re: [PATCH 1/3] PCI: dra7xx: Fix clock enable leak on probe failure

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

Pre-existing issues:
- [High] Missing cleanup of custom INTx IRQ domain and chained handlers on probe failure paths.
- [High] Missing `.remove` callback in a loadable module driver leaves hardware active and leaks resources, leading to Use-After-Free.
--

--- Patch [1]: [PATCH 1/3] PCI: dra7xx: Fix clock enable leak on probe failure ---
commit 949827a7f8e6ad6326095148f4f52df1a75fedb1
Author: Felix Gu <[email protected]>

PCI: dra7xx: Fix clock enable leak on probe failure

dra7xx_pcie_probe() enables the optional clock but never disables it
when probing fails afterwards.

Switch to devm_clk_get_optional_enabled(), which disables the clock
automatically on failure.

Fixes: 5af9405397bf ("PCI: dra7xx: Get an optional clock")
Signed-off-by: Felix Gu <[email protected]>

> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index 6ae5b27e27b32..d43253f7dea65 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -732,15 +732,11 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
>  	if (!link)
>  		return -ENOMEM;
>  
> -	dra7xx->clk = devm_clk_get_optional(dev, NULL);
> +	dra7xx->clk = devm_clk_get_optional_enabled(dev, NULL);
>  	if (IS_ERR(dra7xx->clk))
>  		return dev_err_probe(dev, PTR_ERR(dra7xx->clk),
>  				     "clock request failed");
>  
> -	ret = clk_prepare_enable(dra7xx->clk);
> -	if (ret)
> -		return ret;
> -

[ ... ]

>  	switch (mode) {
>  	case DW_PCIE_RC_TYPE:
>  		if (!IS_ENABLED(CONFIG_PCI_DRA7XX_HOST)) {
>  			ret = -ENODEV;
>  			goto err_gpio;
>  		}
>  
>  		dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE,
>  				   DEVICE_TYPE_RC);
>  
>  		ret = dra7xx_pcie_unaligned_memaccess(dev);
>  		if (ret)
>  			dev_err(dev, "WA for Errata i870 not applied\n");
>  
>  		ret = dra7xx_add_pcie_port(dra7xx, pdev);
>  		if (ret < 0)
>  			goto err_gpio;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this error path
permanently leak the custom IRQ domain?

If dra7xx_add_pcie_port() fails during its internal dw_pcie_host_init() call,
it returns an error without cleaning up the IRQ domain and chained handler it
just created via dra7xx_pcie_init_irq_domain().

Since the probe error path goes straight to err_gpio:, we skip err_deinit:
and the domain is never removed. Because the pci and dra7xx structures are
devm-allocated, they are automatically freed on probe failure.

If the shared IRQ fires afterwards, won't the dangling chained handler
dereference the freed pointer and cause a panic?

[ ... ]

>  static struct platform_driver dra7xx_pcie_driver = {
>  	.probe = dra7xx_pcie_probe,
>  	.driver = {
>  		.name	= "dra7-pcie",
>  		.of_match_table = of_dra7xx_pcie_match,
>  		.suppress_bind_attrs = true,
>  		.pm	= &dra7xx_pcie_pm_ops,
>  	},
>  	.shutdown = dra7xx_pcie_shutdown,
>  };
>  module_platform_driver(dra7xx_pcie_driver);

[Severity: High]
This is a pre-existing issue, but does this driver need a .remove callback?

This driver can be built as a loadable module, but lacks a .remove callback.
When the module is unloaded, the driver core will unbind the device and
devres will release memory and unmap IO.

However, since there is no explicit teardown, pm_runtime_disable() is
skipped, the PHY is never disabled, dw_pcie_host_deinit() is not called,
and the custom IRQ domain and chained handlers are left active.

Does this guarantee a use-after-free when the chained interrupt handler
later accesses the devres-freed memory?

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