Re: [PATCH 3/3] ARM: dts: omap: dra7: Use new property name ti,crossbar-irqs-reserved

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] crossbar_domain_free() uses the crossbar source index (d->hwirq) to index into the GIC SPI array (cb->irq_map) and register offsets array, causing an out-of-bounds write and hardware misconfiguration.
- [High] allocate_gic_irq() modifies cb->irq_map[i] on its error path without holding the required cb->lock.
--

commit 8a92634d55db2a355eaab894b32aa94e0464f39a
Author: Bhargav Joshi <j.bhargav.u@gmail.com>

ARM: dts: omap: dra7: Use new property name ti,crossbar-irqs-reserved
    
Property name for irq-crossbar 'ti,irqs-reserved' is updated to
'ti,crossbar-irqs-reserved' by new binding conversion.

> diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> index 711ce4c31bb1f..80bee3db3a532 100644
> --- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> @@ -866,7 +866,7 @@ crossbar_mpu: crossbar@4a002a48 {
>  			ti,max-irqs = <160>;
>  			ti,max-crossbar-sources = <MAX_SOURCES>;
>  			ti,reg-size = <2>;
> -			ti,irqs-reserved = <0 1 2 3 5 6 131 132>;
> +			ti,crossbar-irqs-reserved = <0 1 2 3 5 6 131 132>;
>  			ti,irqs-skip = <10 133 139 140>;
>  			ti,irqs-safe-map = <0>;
>  		};

While reviewing this patch, I noticed a couple of pre-existing issues in
drivers/irqchip/irq-crossbar.c:

drivers/irqchip/irq-crossbar.c
>  static int allocate_gic_irq(struct irq_domain *domain, unsigned int virq,
>                              irq_hw_number_t hwirq)
>  {
[ ... ]
>  	raw_spin_lock(&cb->lock);
>  	for (i = cb->int_max - 1; i >= 0; i--) {
>  		if (cb->irq_map[i] == IRQ_FREE) {
>  			cb->irq_map[i] = hwirq;
>  			break;
>  		}
>  	}
>  	raw_spin_unlock(&cb->lock);
>  
>  	if (i < 0)
>  		return -ENODEV;
[ ... ]
>  	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
>  	if (err)
>  		cb->irq_map[i] = IRQ_FREE;

[Severity: High]
This is a pre-existing issue, but does this error path modify cb->irq_map[i]
without holding cb->lock?

Earlier in the function, cb->lock is acquired to protect accesses to the
shared cb->irq_map array. Resetting cb->irq_map[i] to IRQ_FREE here without
re-acquiring the lock appears to create a data race with concurrent
allocations or frees on other CPUs.

>  	else
>  		cb->write(i, hwirq);
>  
>  	return err;
>  }

[ ... ]

>  static void crossbar_domain_free(struct irq_domain *domain, unsigned int virq,
>  				 unsigned int nr_irqs)
>  {
>  	int i;
>  
>  	raw_spin_lock(&cb->lock);
>  	for (i = 0; i < nr_irqs; i++) {
>  		struct irq_data *d = irq_domain_get_irq_data(domain, virq + i);
>  
>  		irq_domain_reset_irq_data(d);
>  		cb->irq_map[d->hwirq] = IRQ_FREE;
>  		cb->write(d->hwirq, cb->safe_map);

[Severity: Critical]
This is a pre-existing issue, but does crossbar_domain_free() use the wrong
index when freeing the map?

In allocate_gic_irq(), the allocated GIC SPI index 'i' (bounded by
cb->int_max = 160) is used to index cb->irq_map and cb->register_offsets,
while the crossbar source index 'hwirq' (bounded by
cb->max_crossbar_sources = 400) is stored as the value.

Here, crossbar_domain_free() uses 'd->hwirq' (the crossbar source index) as
the index for cb->irq_map and cb->write(), rather than the GIC SPI index.
Since the source index can greatly exceed the bounds of the array, could
this lead to out-of-bounds memory writes and out-of-bounds MMIO writes to
hardware registers?

>  	}
>  	raw_spin_unlock(&cb->lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-crossbar-v1-0-f67f7cb9ee50@gmail.com?part=3