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

[sashiko-bot@kernel.org]

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