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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The err_cleanup_intx error path destroys the INTx IRQ domain before clearing the active chained handlers, creating a use-after-free race.

Pre-existing issues:
- [High] rzg3s_pcie_intx_irq_handler() incorrectly assumes Linux virtual IRQ numbers are contiguous, which can lead to out-of-bounds domain mapping lookups and interrupt storms.
--

--- Patch [3]: [PATCH v2 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling ---
commit 895e108af15957f16a4f68cbf1072bd988ee57dc
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

PCI: rzg3s: Fix IRQ domain initialization error handling

This commit attempts to fix a resource leak by unwinding chained handler
registrations on error paths in rzg3s_pcie_init_irqdomain(). It also adds
teardown logic to clear the handlers, mirroring the initialization sequence.

Cc: stable@vger.kernel.org

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index f51b5d7c19983..70f1faf7e4227 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c

[ ... ]

> @@ -1006,22 +1006,35 @@ static const struct irq_domain_ops rzg3s_pcie_intx_domain_ops = {
[ ... ]
>  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 err_cleanup_intx;
> +		}
>  
>  		host->intx_irqs[i] = irq;
>  		irq_set_chained_handler_and_data(irq,
>  						 rzg3s_pcie_intx_irq_handler,
>  						 host);
>  	}

[Severity: High]
This is a pre-existing issue, but does this code assume that the Linux virtual
IRQ numbers returned by platform_get_irq_byname() are always contiguous?

Looking at rzg3s_pcie_intx_irq_handler():

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_intx_irq_handler() {
    ...
    unsigned int irq = irq_desc_get_irq(desc);
    u32 intx = irq - host->intx_irqs[0];

    chained_irq_enter(chip, desc);
    generic_handle_domain_irq(host->intx_domain, intx);
    ...
}

If the dynamically allocated virtual IRQs are not strictly sequential, could
this calculation yield an out-of-bounds index for generic_handle_domain_irq()?

If the mapping fails to resolve, could the incoming level-triggered interrupt
remain unacknowledged, leading to an interrupt storm?

[ ... ]

>  	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 err_cleanup_intx;
>  	}
>  
>  	return 0;
> +
> +err_cleanup_intx:
> +	if (host->intx_domain)
> +		irq_domain_remove(host->intx_domain);
> +	rzg3s_pcie_clear_intx_handlers(host, i);

[Severity: High]
Is there a race condition in this error path?

If MSI initialization fails, irq_domain_remove(host->intx_domain) is called
before rzg3s_pcie_clear_intx_handlers().

Since irq_set_chained_handler_and_data() was already called earlier, the
interrupts are unmasked and active.

If a hardware INTx interrupt fires before the handlers are cleared, could
rzg3s_pcie_intx_irq_handler() run and dereference the freed host->intx_domain
via generic_handle_domain_irq(), causing a use-after-free?

Should rzg3s_pcie_clear_intx_handlers() be called before destroying the IRQ
domain?

> +
> +	return ret;
>  }

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