Re: [PATCH 1/2] irqchip: crossbar: Fix out-of-bounds access in crossbar_domain_free()

[Thomas Gleixner <tglx@kernel.org>]

On Wed, Jun 10 2026 at 02:26, Bhargav Joshi wrote:

$Subject: irqchip/crossbar: ....

> crossbar_domain_free() uses 'd->hwirq' (crossbar source index which can
> go up to 0 to 399) as the index for cb->irq_map and cb->write(), rather
> than the GIC SPI index. This can cause out of out-of-bounds write. but
> irq_domain_reset_irq_data() which zeros d->hwirq is called before
> d->hwirq is read. subsequent accesses use hwirq=0 which is always
> in-bounds but writes to the wrong slot.

So the subject line is misleading as there is no out of bounds access at
all. It's not helpful to make claims which are wrong and then not
explaining what the consequences are.

Something like this:

  irqchip/crossbar: Use correct index in crossbar_domain_free()

  crossbar_domain_free() resets the domain data and then uses the nulled
  out data::hwirq member as index to reset the irq_map[] entry and to
  write the relevant crossbar register with a safe entry. That means it
  never frees the correct index and keeps the crossbar register
  connection to the source interrupt active.

  If it would not reset the domain data, then this would be even worse
  as data::hwirq holds the source interrupt number, but both the map and
  register index need the corresponding GIC SPI number and not the
  source interrupt number. This might even result in an out of bounds
  access as the source interrupt number can be higher than the maximal
  index space.

> Fix this by using the GIC SPI index from the parent domain's irq_data,
> moving the reset after cleanup.

The ordering of the reset is not relevant at all once the proper index
is used.

> Fixes: 783d31863fb82 ("irqchip: crossbar: Convert dra7 crossbar to stacked domains")
>

Pointless newline.

> Signed-off-by: Bhargav Joshi <j.bhargav.u@gmail.com>
> ---
>  drivers/irqchip/irq-crossbar.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index cd1134101ace..6a4718be0c58 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -158,9 +158,9 @@ static void crossbar_domain_free(struct irq_domain *domain, unsigned int virq,
>  	for (i = 0; i < nr_irqs; i++) {
>  		struct irq_data *d = irq_domain_get_irq_data(domain, virq + i);
>  
> +		cb->irq_map[d->parent_data->hwirq - GIC_IRQ_START] = IRQ_FREE;
> +		cb->write(d->parent_data->hwirq - GIC_IRQ_START, cb->safe_map);

This lacks a comment explaining why this needs to access
parent_data->hwirq and what that contains.

>  		irq_domain_reset_irq_data(d);
> -		cb->irq_map[d->hwirq] = IRQ_FREE;
> -		cb->write(d->hwirq, cb->safe_map);
>  	}
>  	raw_spin_unlock(&cb->lock);
>  }