[PATCH] PCI: xilinx-xdma-pl: fix refcount leak in xilinx_pl_dma_pcie_init_irq_domain()

[PATCH] PCI: xilinx-xdma-pl: fix refcount leak in xilinx_pl_dma_pcie_init_irq_domain()

[Wentao Liang]

of_get_child_by_name() acquires a reference on the returned device node,
which must be released with of_node_put() after use. In
xilinx_pl_dma_pcie_init_irq_domain(), the reference is properly released
on the success path, but error paths that return directly fail to do so,
leading to a reference count leak.

Additionally, when the INTx domain creation or MSI initialization fails,
the already created PL DMA IRQ domain is not removed, causing another
memory leak.

Fix this by using goto-based error handling:
- Add a common 'out_put_node' label to release the node reference.
- Add 'out_remove_pldma' to delete the PL DMA domain before releasing
  the node.
- Ensure all error paths jump to the appropriate cleanup labels.

Cc: stable@vger.kernel.org
Fixes: 8d786149d78c ("PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver")
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/pci/controller/pcie-xilinx-dma-pl.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c
index b037c8f315e4..e7a83a758874 100644
--- a/drivers/pci/controller/pcie-xilinx-dma-pl.c
+++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c
@@ -580,8 +580,10 @@ static int xilinx_pl_dma_pcie_init_irq_domain(struct pl_dma_pcie *port)
 
 	port->pldma_domain = irq_domain_create_linear(of_fwnode_handle(pcie_intc_node), 32,
 						      &event_domain_ops, port);
-	if (!port->pldma_domain)
-		return -ENOMEM;
+	if (!port->pldma_domain) {
+		ret = -ENOMEM;
+		goto out_put_node;
+	}
 
 	irq_domain_update_bus_token(port->pldma_domain, DOMAIN_BUS_NEXUS);
 
@@ -589,7 +591,8 @@ static int xilinx_pl_dma_pcie_init_irq_domain(struct pl_dma_pcie *port)
 						     &intx_domain_ops, port);
 	if (!port->intx_domain) {
 		dev_err(dev, "Failed to get a INTx IRQ domain\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_remove_pldma;
 	}
 
 	irq_domain_update_bus_token(port->intx_domain, DOMAIN_BUS_WIRED);
@@ -597,13 +600,20 @@ static int xilinx_pl_dma_pcie_init_irq_domain(struct pl_dma_pcie *port)
 	ret = xilinx_pl_dma_pcie_init_msi_irq_domain(port);
 	if (ret != 0) {
 		irq_domain_remove(port->intx_domain);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_remove_pldma;
 	}
 
 	of_node_put(pcie_intc_node);
 	raw_spin_lock_init(&port->lock);
 
 	return 0;
+
+out_remove_pldma:
+	irq_domain_remove(port->pldma_domain);
+out_put_node:
+	of_node_put(pcie_intc_node);
+	return ret;
 }
 
 static int xilinx_pl_dma_pcie_setup_irq(struct pl_dma_pcie *port)
-- 
2.34.1

Re: [PATCH] PCI: xilinx-xdma-pl: fix refcount leak in xilinx_pl_dma_pcie_init_irq_domain()

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling `irq_domain_remove(port->intx_domain)` after `xilinx_pl_dma_pcie_init_msi_irq_domain(port)` fails causes a NULL pointer dereference.
- [High] The probe function silently ignores the return value of `xilinx_pl_dma_pcie_setup_irq()`, allowing the device to probe successfully even if IRQ setup fails.
- [High] `port->pldma_domain` and `msi->bitmap` are leaked when `pci_host_probe()` fails because `xilinx_pl_dma_pcie_free_irq_domains()` neglects to free them.
- [High] Missing `irq_dispose_mapping()` causing mapping leaks and potential WARN_ON() on domain removal.
- [High] Use-After-Free or NULL pointer dereference of `msi->dev_domain` due to early registration of the MSI interrupt handler.
- [Medium] Leak of `port->cfg` if `devm_ioremap_resource` fails in `xilinx_pl_dma_pcie_parse_dt`.
- [Medium] Double initialization of `port->lock` raw spinlock.
--

commit 255f95990ff953845972471921209ec22fc3d8f6
Author: Wentao Liang <vulab@iscas.ac.cn>
Subject: PCI: xilinx-xdma-pl: fix refcount leak in xilinx_pl_dma_pcie_init_irq_domain()

This commit fixes an OF node reference leak in the error paths of
xilinx_pl_dma_pcie_init_irq_domain(). It introduces goto labels to ensure
proper cleanup of both the OF node and the PL DMA domain.

> diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c
> index b037c8f315e46..e7a83a758874b 100644
> --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c
> +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c
[ ... ]
> @@ -597,13 +600,20 @@ static int xilinx_pl_dma_pcie_init_irq_domain(struct pl_dma_pcie *port)
>  	ret = xilinx_pl_dma_pcie_init_msi_irq_domain(port);
>  	if (ret != 0) {
>  		irq_domain_remove(port->intx_domain);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto out_remove_pldma;
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path cause a NULL pointer
dereference?

If xilinx_pl_dma_pcie_init_msi_irq_domain() fails, it cleans up by calling
xilinx_pl_dma_pcie_free_irq_domains(). This function frees port->intx_domain
and sets it to NULL. When execution returns here, irq_domain_remove() is
called on the already-nullified port->intx_domain, which dereferences the
pointer unconditionally.

>  
>  	of_node_put(pcie_intc_node);
>  	raw_spin_lock_init(&port->lock);
>  
>  	return 0;

[Severity: Medium]
This is a pre-existing issue, but is the port->lock initialized twice?

It appears xilinx_pl_dma_pcie_init_msi_irq_domain() already calls
raw_spin_lock_init(&port->lock) right before returning successfully. Calling
it again here could destroy the active lock state or trigger lockdep
warnings if an interrupt handler has already acquired it.


[Severity: High]
This is a pre-existing issue, but does xilinx_pl_dma_pcie_free_irq_domains()
leak port->pldma_domain and msi->bitmap?

When pci_host_probe() fails in xilinx_pl_dma_pcie_probe(), the cleanup path
calls xilinx_pl_dma_pcie_free_irq_domains():

drivers/pci/controller/pcie-xilinx-dma-pl.c:xilinx_pl_dma_pcie_free_irq_domains() {
    ...
    if (port->intx_domain) {
        irq_domain_remove(port->intx_domain);
        port->intx_domain = NULL;
    }
    ...
}

This teardown helper removes intx_domain and msi->dev_domain, but completely
misses port->pldma_domain. Additionally, msi->bitmap is allocated in
xilinx_pl_dma_pcie_init_msi_irq_domain() but does not seem to be freed
anywhere in the driver.


[Severity: High]
This is a pre-existing issue, but does the probe function ignore IRQ setup
failures?

In xilinx_pl_dma_pcie_probe():

drivers/pci/controller/pcie-xilinx-dma-pl.c:xilinx_pl_dma_pcie_probe() {
    ...
    err = xilinx_pl_dma_pcie_setup_irq(port);
    
    bridge->sysdata = port;
    bridge->ops = &xilinx_pl_dma_pcie_ops.pci_ops;
    
    err = pci_host_probe(bridge);
    ...
}

The result of xilinx_pl_dma_pcie_setup_irq() is immediately overwritten by
pci_host_probe(). If IRQ mapping or requesting fails, the driver proceeds
anyway, which might leave the hardware in a broken state without functional
interrupts.


[Severity: High]
This is a pre-existing issue, but are we missing irq_dispose_mapping() for
the IRQs mapped during setup?

In xilinx_pl_dma_pcie_setup_irq():

drivers/pci/controller/pcie-xilinx-dma-pl.c:xilinx_pl_dma_pcie_setup_irq() {
    ...
    irq = irq_create_mapping(port->pldma_domain, i);
    ...
}

The driver creates these mappings but never provides corresponding
irq_dispose_mapping() calls in error paths or driver teardown. If the domain
is ever removed, irq_domain_remove() might trigger a WARN_ON because the
undisposed mappings remain in the radix tree.


[Severity: Medium]
This is a pre-existing issue, but does xilinx_pl_dma_pcie_parse_dt() leak
port->cfg?

drivers/pci/controller/pcie-xilinx-dma-pl.c:xilinx_pl_dma_pcie_parse_dt() {
    ...
    port->reg_base = devm_ioremap_resource(dev, res);
    if (IS_ERR(port->reg_base))
        return PTR_ERR(port->reg_base);
    ...
}

If devm_ioremap_resource() fails, the function returns immediately without
calling pci_ecam_free(port->cfg), leaving the previously allocated ECAM
structure stranded.


[Severity: High]
This is a pre-existing issue, but could early registration of the MSI
interrupt handlers cause a NULL pointer dereference or use-after-free?

In xilinx_pl_dma_pcie_parse_dt(), xilinx_request_msi_irq() is called:

drivers/pci/controller/pcie-xilinx-dma-pl.c:xilinx_pl_dma_pcie_parse_dt() {
    ...
    err = xilinx_request_msi_irq(port);
    ...
}

This registers the MSI interrupt handlers before the IRQ domains are created.
If an interrupt fires immediately, xilinx_pl_dma_pcie_msi_handler_low() reads
msi->dev_domain (which is still NULL) and passes it to irq_find_mapping().

Also, if xilinx_pl_dma_pcie_init_msi_irq_domain() fails later and frees
msi->dev_domain, an interrupt firing during that window could result in a
use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616140949.1686840-1-vulab@iscas.ac.cn?part=1