Re: [PATCH v3 1/3] PCI: rzg3s: Disable refclk on probe failure

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

Pre-existing issues:
- [High] System suspend is unconditionally aborted due to incorrect pm_runtime_put_sync() usage in the suspend_noirq phase.
- [High] Use-after-free vulnerability due to missing chained IRQ handler cleanup on probe error paths.
--

--- Patch [1]: [PATCH v3 1/3] PCI: rzg3s: Disable refclk on probe failure ---
commit 7f97e9f3db5d36293fbc8938e90e7f9e5ceaa52c
Author: Lad Prabhakar <[email protected]>

PCI: rzg3s: Disable refclk on probe failure

rzg3s_pcie_host_init_port() enables the PCIe reference clock using
clk_prepare_enable(), but if rzg3s_pcie_probe() subsequently fails, for
example because pci_host_probe() returns an error, the probe error path
only releases the clock handle with clk_put().

Balance clk_prepare_enable() by calling clk_disable_unprepare() in the
probe cleanup path before releasing the clock, ensuring the reference
clock is properly disabled on probe failure.

Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: [email protected]

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 66f687304c1cb..bd6471a7fb7a1 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -1762,6 +1762,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  
>  host_probe_teardown:
> +	clk_disable_unprepare(host->port.refclk);
>  	rzg3s_pcie_teardown_irqdomain(host);

[Severity: High]
This is a pre-existing issue, but does this teardown path leave dangling
pointers in the chained IRQ handlers?

Looking at rzg3s_pcie_init_irqdomain(), chained handlers are registered
with the devres-managed host pointer:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_init_irqdomain() {
    ...
    irq_set_chained_handler_and_data(irq,
                                     rzg3s_pcie_intx_irq_handler,
                                     host);
    ...
}

But rzg3s_pcie_teardown_irqdomain() does not clear them:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_teardown_irqdomain() {
    if (IS_ENABLED(CONFIG_PCI_MSI))
        rzg3s_pcie_teardown_msi(host);
    irq_domain_remove(host->intx_domain);
}

If the probe fails and the host structure is freed, wouldn't a subsequent
spurious interrupt on any INTx line trigger a use-after-free when the chained
handler dereferences the freed host pointer?

Should this call irq_set_chained_handler_and_data() with NULL to clean up?

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a potential issue with the suspend sequence:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_suspend_noirq() {
    ...
    ret = pm_runtime_put_sync(dev);
    if (ret)
        return ret;
    ...
}

Will this unconditionally abort the system suspend?

During the system suspend transition, the PM core disables runtime PM in the
suspend_late phase.

When suspend_noirq is later called, wouldn't pm_runtime_put_sync() return
-EACCES since runtime PM is disabled, causing the entire system suspend to
fail?

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