[PATCH v3 0/3] PCI: rzg3s: Fix issues in cleanup paths

[PATCH v3 0/3] PCI: rzg3s: Fix issues in cleanup paths

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi all,

This series fixes a few issues in the cleanup paths of the Renesas RZ/G3S
PCIe host controller driver. The fixes include:
- Disabling the PCIe reference clock on probe failure.
- Propagating the original error code from platform_get_irq_byname() to
allow probe deferral.
- Unwinding chained handler registration on error in
rzg3s_pcie_init_irqdomain() and clearing chained handlers in
rzg3s_pcie_teardown_irqdomain() to avoid leaving stale handler and data
pointers attached to the parent interrupts.


v2->v3:
- Added rzg3s_pcie_teardown_intx() helper to remove the irqdomain
  and clear chained handlers.
- Updated commit message for patch 3.

Cheers,
Prabhakar

Lad Prabhakar (3):
  PCI: rzg3s: Disable refclk on probe failure
  PCI: rzg3s: Propagate platform_get_irq_byname() errors
  PCI: rzg3s: Fix IRQ domain initialization error handling

 drivers/pci/controller/pcie-rzg3s-host.c | 51 +++++++++++++++++-------
 1 file changed, 37 insertions(+), 14 deletions(-)

-- 
2.43.0

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

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3:
- No change

v1->v2:
- Added RB tag
---
 drivers/pci/controller/pcie-rzg3s-host.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index a1a78bdb7af1..01488660ab08 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1897,6 +1897,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);
 	host->data->config_deinit(host);
 rpm_put:
-- 
2.43.0

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

[sashiko-bot]

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 <prabhakar.mahadev-lad.rj@bp.renesas.com>

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: stable@vger.kernel.org

> 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/20260811200013.185956-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1

[PATCH v3 2/3] PCI: rzg3s: Propagate platform_get_irq_byname() errors

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

platform_get_irq_byname() may return specific error codes such as
-EPROBE_DEFER, but rzg3s_pcie_init_irqdomain() replaces all failures
with -EINVAL.

This breaks probe deferral by preventing the driver core from retrying
probe once the interrupt provider becomes available.

Propagate the original error code instead.

Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3:
- No change

v1->v2:
- Added RB tag
---
 drivers/pci/controller/pcie-rzg3s-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 01488660ab08..f51b5d7c1998 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1019,7 +1019,7 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 
 		irq = platform_get_irq_byname(pdev, irq_name);
 		if (irq < 0)
-			return dev_err_probe(dev, -EINVAL,
+			return dev_err_probe(dev, irq,
 					     "Failed to parse and map INT%c IRQ\n",
 					     'A' + i);
 
-- 
2.43.0

Re: [PATCH v3 2/3] PCI: rzg3s: Propagate platform_get_irq_byname() errors

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cleanup of chained IRQ handlers triggers a Use-After-Free panic on probe failure.
- [High] Logic error due to false assumption of contiguous virtual IRQ numbers leads to unhandled INTx interrupts.
--

--- Patch [2]: [PATCH v3 2/3] PCI: rzg3s: Propagate platform_get_irq_byname() errors ---
commit 398dc4c2681ec621a5ea3ee51113abc78f26854a
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

PCI: rzg3s: Propagate platform_get_irq_byname() errors

platform_get_irq_byname() may return specific error codes such as
-EPROBE_DEFER, but rzg3s_pcie_init_irqdomain() replaces all failures
with -EINVAL. This commit propagates the original error code instead to
allow probe deferral.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index bd6471a7fb7a1..92371310d313b 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -975,7 +975,7 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
>  
>  		irq = platform_get_irq_byname(pdev, irq_name);
>  		if (irq < 0)
> -			return dev_err_probe(dev, -EINVAL,
> +			return dev_err_probe(dev, irq,

[Severity: High]
This is a pre-existing issue, but does this error path leave chained handlers
registered?

If rzg3s_pcie_init_irqdomain() successfully maps INTA but fails on a
subsequent INTx setup like an INTB probe deferral, returning directly leaves
the earlier chained handlers active. This same unhandled registration happens 
if irq_domain_create_linear() fails later, or if the teardown function 
omits unregistering the INTx chained handlers.

Since the device probe fails, the devres framework will free the host
structure. If the unhandled chained interrupt fires later, it will
dereference the freed host pointer, leading to a use-after-free panic:

rzg3s_pcie_intx_irq_handler()
    struct rzg3s_pcie_host *host = irq_desc_get_handler_data(desc);
    // UAF on host access

>  					     "Failed to parse and map INT%c IRQ\n",
>  					     'A' + i);
>  

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the INTx 
handling in this file, I noticed a potential issue in 
rzg3s_pcie_intx_irq_handler().

Is the calculation for the INTx index assuming contiguous virtual IRQ numbers?

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_intx_irq_handler() {
    ...
    u32 intx = irq - host->intx_irqs[0];
    ...
    generic_handle_domain_irq(host->intx_domain, intx);
}

If the OS allocates non-contiguous virqs (for example, due to concurrent IRQ
allocation or ID reuse by the IDR), this calculation will yield an invalid
out-of-bounds index or underflow.

Can this cause the generic handler lookup to fail? If so, leaving the INTx
interrupt unhandled might lead to an IRQ storm that permanently disables
the interrupt line and breaks downstream PCIe devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811200013.185956-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=2

[PATCH v3 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

rzg3s_pcie_init_irqdomain() installs chained handlers for the INTx
parent interrupts before creating the INTx irqdomain and initializing
MSI support.

If any subsequent step fails, such as obtaining an INTx IRQ,
creating the irqdomain, or initializing MSI, the error path returns
without removing any chained handlers that were already installed.
This leaves stale handler and data pointers attached to the parent
interrupts.

Add a helper to tear down the INTx IRQ setup, including removing the IRQ
domain and clearing the chained handlers. Use it to unwind partially
completed initialization and from the normal IRQ domain teardown path.

Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3:
- Added rzg3s_pcie_teardown_intx() helper to remove the irqdomain
  and clear chained handlers.
- Updated commit message.

v1->v2:
- Restricted the code to max 80 columns.
- Moved calling irq_domain_remove() under err_cleanup_intx label.
---
 drivers/pci/controller/pcie-rzg3s-host.c | 50 +++++++++++++++++-------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index f51b5d7c1998..3f7393c49a08 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1006,22 +1006,38 @@ static const struct irq_domain_ops rzg3s_pcie_intx_domain_ops = {
 	.xlate = irq_domain_xlate_onetwocell,
 };
 
+static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host,
+				     int count)
+{
+	while (--count >= 0) {
+		irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
+						 NULL);
+	}
+
+	if (host->intx_domain)
+		irq_domain_remove(host->intx_domain);
+}
+
 static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 {
 	struct device *dev = host->dev;
 	struct platform_device *pdev = to_platform_device(dev);
+	int i, ret;
 
-	for (int i = 0; i < PCI_NUM_INTX; i++) {
+	for (i = 0; i < PCI_NUM_INTX; i++) {
 		char irq_name[5] = {0};
 		int irq;
 
 		scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
 
 		irq = platform_get_irq_byname(pdev, irq_name);
-		if (irq < 0)
-			return dev_err_probe(dev, irq,
-					     "Failed to parse and map INT%c IRQ\n",
-					     'A' + i);
+		if (irq < 0) {
+			ret = irq;
+			dev_err_probe(dev, ret,
+				      "Failed to parse and map INT%c IRQ\n",
+				      'A' + i);
+			goto teardown_intx;
+		}
 
 		host->intx_irqs[i] = irq;
 		irq_set_chained_handler_and_data(irq,
@@ -1033,21 +1049,27 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 						     PCI_NUM_INTX,
 						     &rzg3s_pcie_intx_domain_ops,
 						     host);
-	if (!host->intx_domain)
-		return dev_err_probe(dev, -EINVAL,
-				     "Failed to add irq domain for INTx IRQs\n");
+	if (!host->intx_domain) {
+		ret = -EINVAL;
+		dev_err_probe(dev, ret,
+			      "Failed to add irq domain for INTx IRQs\n");
+		goto teardown_intx;
+	}
 	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		int ret = rzg3s_pcie_init_msi(host);
+		ret = rzg3s_pcie_init_msi(host);
 
-		if (ret) {
-			irq_domain_remove(host->intx_domain);
-			return ret;
-		}
+		if (ret)
+			goto teardown_intx;
 	}
 
 	return 0;
+
+teardown_intx:
+	rzg3s_pcie_teardown_intx(host, i);
+
+	return ret;
 }
 
 static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
@@ -1055,7 +1077,7 @@ static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		rzg3s_pcie_teardown_msi(host);
 
-	irq_domain_remove(host->intx_domain);
+	rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
 }
 
 static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
-- 
2.43.0

Re: [PATCH v3 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling

[sashiko-bot]

> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> rzg3s_pcie_init_irqdomain() installs chained handlers for the INTx
> parent interrupts before creating the INTx irqdomain and initializing
> MSI support.
> 
> If any subsequent step fails, such as obtaining an INTx IRQ,
> creating the irqdomain, or initializing MSI, the error path returns
> without removing any chained handlers that were already installed.
> This leaves stale handler and data pointers attached to the parent
> interrupts.
> 
> Add a helper to tear down the INTx IRQ setup, including removing the IRQ
> domain and clearing the chained handlers. Use it to unwind partially
> completed initialization and from the normal IRQ domain teardown path.
> 
> Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811200013.185956-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=3