[PATCH] PCI: starfive: Fix resource leaks on error paths in host_init()

[PATCH] PCI: starfive: Fix resource leaks on error paths in host_init()

[Ali Tariq]

starfive_pcie_host_init() acquires the PHY, clocks/resets, and an
optional regulator in sequence, but does not correctly unwind these
resources when a later step fails.

If starfive_pcie_clk_rst_init() fails after the PHY has already been
successfully enabled, the function returns directly without disabling
the PHY, leaking it and leaving it powered.

If regulator_enable() fails for the optional vpcie3v3 regulator, the
failure is only logged; the function falls through and returns
success, leaving the driver believing the regulator is enabled while
continuing to configure PCIe hardware that may be unpowered. This
also leaves the clocks and PHY enabled with nothing to clean them up.

Disable the PHY on the clk/reset failure path, and disable the
clocks/resets and PHY, then return the error, if the regulator fails
to enable.

Build-tested and boot-tested on StarFive VisionFive 2 v1.2A

Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
 drivers/pci/controller/plda/pcie-starfive.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index ba8ef7842e35..156b31eef868 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -303,13 +303,19 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 			   STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ);
 
 	ret = starfive_pcie_clk_rst_init(pcie);
-	if (ret)
+	if (ret) {
+		starfive_pcie_disable_phy(pcie);
 		return ret;
+	}
 
 	if (pcie->vpcie3v3) {
 		ret = regulator_enable(pcie->vpcie3v3);
-		if (ret)
+		if (ret) {
 			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
+			starfive_pcie_clk_rst_deinit(pcie);
+			starfive_pcie_disable_phy(pcie);
+			return ret;
+		}
 	}
 
 	if (pcie->reset_gpio)
-- 
2.34.1

Re: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init()

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free of IRQ domains during driver unbind or when probe fails after interrupt initialization.
--

--- Patch [1]: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init() ---
commit 64603196409722c49e238cd53d9d6a6b7efd0b22
Author: Ali Tariq <alitariq45892@gmail.com>

PCI: starfive: Fix resource leaks on error paths in host_init()

This commit unwinds previously acquired PHY and clock/reset resources in
starfive_pcie_host_init() when subsequent initialization steps or regulator
enablement fails.

> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
> index 298036c3e7f94..86b248a1ea758 100644
> --- a/drivers/pci/controller/plda/pcie-starfive.c
> +++ b/drivers/pci/controller/plda/pcie-starfive.c
[ ... ]
> @@ -303,13 +303,19 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
>  			   STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ);
>  
>  	ret = starfive_pcie_clk_rst_init(pcie);
> -	if (ret)
> +	if (ret) {
> +		starfive_pcie_disable_phy(pcie);
>  		return ret;
> +	}
>  
>  	if (pcie->vpcie3v3) {
>  		ret = regulator_enable(pcie->vpcie3v3);
> -		if (ret)
> +		if (ret) {
>  			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
> +			starfive_pcie_clk_rst_deinit(pcie);
> +			starfive_pcie_disable_phy(pcie);
> +			return ret;
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but since we are fixing error paths in the
host initialization, I noticed a potential use-after-free in the related
PLDA host initialization code.

In drivers/pci/controller/plda/pcie-plda-host.c, plda_init_interrupts()
allocates the interrupt using devm_request_irq():

plda_init_interrupts() {
    ...
    ret = devm_request_irq(dev, event_irq,
                           plda_event_handler,
                           0, NULL, port);
    ...
}

However, on device removal or if probe fails, plda_pcie_host_init() calls
plda_pcie_irq_domain_deinit(), which explicitly removes and frees the event
domain:

plda_pcie_host_init() {
    ...
err_probe:
    plda_pcie_irq_domain_deinit(port);
    ...
}

plda_pcie_irq_domain_deinit() {
    ...
    irq_domain_remove(pcie->event_domain);
    ...
}

Because the IRQs are devm-managed, their cleanup is deferred until after the
driver's probe or remove function returns.

When devres eventually calls free_irq(), will the IRQ core attempt to
deactivate the interrupt using the now-freed irq_data->domain pointer?

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

Re: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init()

[Ali Tariq]

This is a separate, unrelated issue from the submitted patch. I will 
work on this and will submit a separate patch.

On 7/14/26 4:53 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Use-after-free of IRQ domains during driver unbind or when probe fails after interrupt initialization.
> --
> 
> --- Patch [1]: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init() ---
> commit 64603196409722c49e238cd53d9d6a6b7efd0b22
> Author: Ali Tariq <alitariq45892@gmail.com>
> 
> PCI: starfive: Fix resource leaks on error paths in host_init()
> 
> This commit unwinds previously acquired PHY and clock/reset resources in
> starfive_pcie_host_init() when subsequent initialization steps or regulator
> enablement fails.
> 
>> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
>> index 298036c3e7f94..86b248a1ea758 100644
>> --- a/drivers/pci/controller/plda/pcie-starfive.c
>> +++ b/drivers/pci/controller/plda/pcie-starfive.c
> [ ... ]
>> @@ -303,13 +303,19 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
>>   			   STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ);
>>   
>>   	ret = starfive_pcie_clk_rst_init(pcie);
>> -	if (ret)
>> +	if (ret) {
>> +		starfive_pcie_disable_phy(pcie);
>>   		return ret;
>> +	}
>>   
>>   	if (pcie->vpcie3v3) {
>>   		ret = regulator_enable(pcie->vpcie3v3);
>> -		if (ret)
>> +		if (ret) {
>>   			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
>> +			starfive_pcie_clk_rst_deinit(pcie);
>> +			starfive_pcie_disable_phy(pcie);
>> +			return ret;
>> +		}
>>   	}
> 
> [Severity: High]
> This is a pre-existing issue, but since we are fixing error paths in the
> host initialization, I noticed a potential use-after-free in the related
> PLDA host initialization code.
> 
> In drivers/pci/controller/plda/pcie-plda-host.c, plda_init_interrupts()
> allocates the interrupt using devm_request_irq():
> 
> plda_init_interrupts() {
>      ...
>      ret = devm_request_irq(dev, event_irq,
>                             plda_event_handler,
>                             0, NULL, port);
>      ...
> }
> 
> However, on device removal or if probe fails, plda_pcie_host_init() calls
> plda_pcie_irq_domain_deinit(), which explicitly removes and frees the event
> domain:
> 
> plda_pcie_host_init() {
>      ...
> err_probe:
>      plda_pcie_irq_domain_deinit(port);
>      ...
> }
> 
> plda_pcie_irq_domain_deinit() {
>      ...
>      irq_domain_remove(pcie->event_domain);
>      ...
> }
> 
> Because the IRQs are devm-managed, their cleanup is deferred until after the
> driver's probe or remove function returns.
> 
> When devres eventually calls free_irq(), will the IRQ core attempt to
> deactivate the interrupt using the now-freed irq_data->domain pointer?
> 

Re: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init()

[Markus Elfring]

…
> Disable the PHY on the clk/reset failure path, and disable the
> clocks/resets and PHY, then return the error, if the regulator fails
> to enable.
…

Please avoid a bit of duplicate source code by using an additional label.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.2-rc3#n526

Regards,
Markus