[PATCH] PCI: aardvark: Disable PHY on probe failures

[PATCH] PCI: aardvark: Disable PHY on probe failures

[Myeonghun Pak]

advk_pcie_setup_phy() initializes and powers on the PHY before the
emulated bridge and IRQ domains are created.  If any of those later
initialization steps or pci_host_probe() fails, probe returns without
powering off or exiting the PHY.

Replace the direct returns with staged cleanup labels.  Remove IRQ
domains in reverse order, release the emulated bridge allocation only
after successful initialization, and disable the PHY on every failure
after advk_pcie_setup_phy() succeeds.  A setup_phy() failure still
returns directly, so its partial internal cleanup is not repeated.

Fixes: 366697018c9a ("PCI: aardvark: Add PHY support")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/pci/controller/pci-aardvark.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ecb81ac73019..2bbdb73cfb45 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1864,28 +1864,25 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	ret = advk_sw_pci_bridge_init(pcie);
 	if (ret) {
 		dev_err(dev, "Failed to register emulated root PCI bridge\n");
-		return ret;
+		goto err_disable_phy;
 	}
 
 	ret = advk_pcie_init_irq_domain(pcie);
 	if (ret) {
 		dev_err(dev, "Failed to initialize irq\n");
-		return ret;
+		goto err_cleanup_bridge;
 	}
 
 	ret = advk_pcie_init_msi_irq_domain(pcie);
 	if (ret) {
 		dev_err(dev, "Failed to initialize irq\n");
-		advk_pcie_remove_irq_domain(pcie);
-		return ret;
+		goto err_remove_irq_domain;
 	}
 
 	ret = advk_pcie_init_rp_irq_domain(pcie);
 	if (ret) {
 		dev_err(dev, "Failed to initialize irq\n");
-		advk_pcie_remove_msi_irq_domain(pcie);
-		advk_pcie_remove_irq_domain(pcie);
-		return ret;
+		goto err_remove_msi_irq_domain;
 	}
 
 	bridge->sysdata = pcie;
@@ -1893,14 +1890,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	bridge->map_irq = advk_pcie_map_irq;
 
 	ret = pci_host_probe(bridge);
-	if (ret < 0) {
-		advk_pcie_remove_rp_irq_domain(pcie);
-		advk_pcie_remove_msi_irq_domain(pcie);
-		advk_pcie_remove_irq_domain(pcie);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_remove_rp_irq_domain;
 
 	return 0;
+
+err_remove_rp_irq_domain:
+	advk_pcie_remove_rp_irq_domain(pcie);
+err_remove_msi_irq_domain:
+	advk_pcie_remove_msi_irq_domain(pcie);
+err_remove_irq_domain:
+	advk_pcie_remove_irq_domain(pcie);
+err_cleanup_bridge:
+	pci_bridge_emul_cleanup(&pcie->bridge);
+err_disable_phy:
+	advk_pcie_disable_phy(pcie);
+	return ret;
 }
 
 static void advk_pcie_remove(struct platform_device *pdev)

-- 
2.50.1

Re: [PATCH] PCI: aardvark: Disable PHY on probe failures

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free in probe error path due to missing hardware interrupt teardown.
- [High] Initialization race allows IRQ handler to execute before IRQ domains are created, leading to NULL pointer dereference or routing failures.
--

commit c3281f5a40eba7b88be1baec66979e22e39980ca
Author: Myeonghun Pak <mhun512@gmail.com>

    PCI: aardvark: Disable PHY on probe failures
    
    advk_pcie_setup_phy() initializes and powers on the PHY before the
    emulated bridge and IRQ domains are created.  If any of those later
    initialization steps or pci_host_probe() fails, probe returns without
    powering off or exiting the PHY.

> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index ecb81ac730199..2bbdb73cfb45b 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1864,28 +1864,25 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	ret = advk_sw_pci_bridge_init(pcie);
>  	if (ret) {
>  		dev_err(dev, "Failed to register emulated root PCI bridge\n");
> -		return ret;
> +		goto err_disable_phy;
>  	}
>  
>  	ret = advk_pcie_init_irq_domain(pcie);

[Severity: High]
This isn't a bug introduced by this patch, but does the current sequence
create an initialization race?

Earlier in advk_pcie_probe(), the driver registers advk_pcie_irq_handler
with IRQF_SHARED:

	ret = devm_request_irq(dev, irq, advk_pcie_irq_handler,
			       IRQF_SHARED | IRQF_NO_THREAD, "advk-pcie",
			       pcie);

It then unmasks hardware interrupts in advk_pcie_setup_hw().

If a shared interrupt fires, or if the hardware asserts an interrupt right
after advk_pcie_setup_hw(), the handler will execute before this software IRQ
domain is allocated. Could this cause generic_handle_domain_irq() to execute
with uninitialized (NULL) domain pointers?

>  	if (ret) {
>  		dev_err(dev, "Failed to initialize irq\n");
> -		return ret;
> +		goto err_cleanup_bridge;
>  	}

[ ... ]

> +err_remove_rp_irq_domain:
> +	advk_pcie_remove_rp_irq_domain(pcie);
> +err_remove_msi_irq_domain:
> +	advk_pcie_remove_msi_irq_domain(pcie);
> +err_remove_irq_domain:
> +	advk_pcie_remove_irq_domain(pcie);
> +err_cleanup_bridge:
> +	pci_bridge_emul_cleanup(&pcie->bridge);
> +err_disable_phy:
> +	advk_pcie_disable_phy(pcie);
> +	return ret;

[Severity: High]
This is a pre-existing issue, but does this error path leave a window
for a use-after-free?

The software IRQ domains are freed here, but hardware interrupts are not
explicitly masked (unlike in advk_pcie_remove()).

Because advk_pcie_irq_handler is registered with devm_request_irq(), it
remains active until after advk_pcie_probe() returns.

If a hardware or shared interrupt fires during this window, will the handler
execute and call generic_handle_domain_irq() with the freed domain pointers?

>  }
>  
>  static void advk_pcie_remove(struct platform_device *pdev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910221725.91980-1-mhun512@gmail.com?part=1