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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free window in INTx IRQ domain error handling path.
- [High] Chained INTx handlers are registered before their dependent IRQ domain is created.
- [High] Unsafe contiguous virtual IRQ assumption leads to incorrect hardware IRQ index calculation.
- [High] Read-Modify-Write operations corrupt Write-1-to-Clear (W1C) interrupt status registers.
--

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

This commit attempts to fix the error path during IRQ domain initialization
by ensuring chained handlers are unregistered if subsequent steps fail. It
also updates the teardown path to clear INTx handlers.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
>  static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
>  {
[ ... ]
>  	for (i = 0; i < PCI_NUM_INTX; i++) {
[ ... ]
>  		host->intx_irqs[i] = irq;
>  		irq_set_chained_handler_and_data(irq,
>  						 rzg3s_pcie_intx_irq_handler,
>  						 host);
>  	}
>  
>  	host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
>  						     PCI_NUM_INTX,
>  						     &rzg3s_pcie_intx_domain_ops,
>  						     host);

[Severity: High]
This is a pre-existing issue, but does this sequence leave a window where
interrupts can be misrouted?

If a hardware INTx interrupt fires immediately after unmasking via
irq_set_chained_handler_and_data(), rzg3s_pcie_intx_irq_handler() will execute
while host->intx_domain is still NULL.

Passing a NULL domain to generic_handle_domain_irq() causes the IRQ core to
fall back to the system's irq_default_domain, which might route spurious
interrupts to unrelated devices.

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

[Severity: High]
This isn't a bug introduced by this patch, but does the error cleanup order
create a use-after-free window?

If rzg3s_pcie_init_msi() fails, the code calls
irq_domain_remove(host->intx_domain) and then jumps to err_cleanup_intx to
clear the handlers.

If a hardware INTx interrupt fires between the domain removal and unregistering
the handler, rzg3s_pcie_intx_irq_handler() could dereference the already freed
host->intx_domain pointer.

[Severity: High]
This is a pre-existing issue in the handler itself. Is it safe to assume
platform IRQs are strictly contiguous?

rzg3s_pcie_intx_irq_handler() calculates the hardware index by subtracting
virtual Linux IRQ numbers:

rzg3s_pcie_intx_irq_handler() {
        ...
        unsigned int irq = irq_desc_get_irq(desc);
        u32 intx = irq - host->intx_irqs[0];
        ...
}

Since platform_get_irq_byname() dynamically requests platform IRQs, they are
not guaranteed to be contiguous. If they are disjoint, this subtraction would
produce an out-of-bounds index for generic_handle_domain_irq().

[Severity: High]
This is another pre-existing issue regarding interrupt acknowledgment. Will
using a read-modify-write operation on a Write-1-to-Clear (W1C) register
silently drop concurrently pending interrupts?

For example, in rzg3s_pcie_msi_irq():

rzg3s_pcie_msi_irq() {
        ...
        /* Clear the MSI */
        rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
                               RZG3S_PCI_PINTRCVIS_MSI,
                               RZG3S_PCI_PINTRCVIS_MSI);
        ...
}

If RZG3S_PCI_PINTRCVIS is a W1C register, reading the current state and
writing it back means any concurrently pending interrupts (read as 1) will be
written back as 1, inadvertently clearing them before they can be handled.

A similar operation happens in rzg3s_pcie_intx_irq_ack().

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