Re: [PATCH v3 2/4] xen/arm: validate IRQs before descriptor lookup

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

On Mon, Sep 14, 2026 at 05:20:44PM +0200, Orzel, Michal wrote:
> 
> 
> On 25-Aug-26 02:30, Volodymyr Babchuk wrote:
> > Hi,
> > 
> > Mykola Kvach <mykola_kvach@xxxxxxxx> writes:
> > 
> >> GICv3 eSPI support makes nr_irqs span the architectural INTID namespace
> >> through ESPI_MAX_INTID, but descriptor storage is sparse. local_irq_desc[]
> >> and irq_desc[] cover INTIDs below NR_IRQS, while espi_desc[] covers eSPIs.
> >> INTIDs 1024 through 4095 have no backing descriptors.
> >>
> >> Validation based only on nr_irqs accepts an INTID in this gap.
> >> __irq_to_desc() then indexes beyond irq_desc[], and callers may lock or
> >> update unrelated Xen memory.
> >>
> >> Reject INTIDs that the GIC reports as unimplemented in setup_irq() before
> >> looking up a descriptor. irq_set_spi_type() can run before the implemented
> >> GIC line counts are available, so validate descriptor-backed ranges there
> >> before looking up a descriptor.
> >>
> >> Assert the regular descriptor bound in __irq_to_desc() so direct callers
> >> cannot silently index the sparse gap in debug builds.
> >>
> >> Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI 
> >> range")
> >> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >> ---
> >> Changes in v3:
> >> - Add the requested bound assertion and retain the SPI-only comment.
> >>
> >> Changes in v2:
> >> - Validate descriptor-backed ranges in irq_set_spi_type().
> >> - Validate implemented GIC lines in setup_irq().
> >> - Preserve is_espi() validation with CONFIG_GICV3_ESPI disabled.
> >> ---
> >>  xen/arch/arm/irq.c | 26 ++++++++++++++++++++++----
> >>  1 file changed, 22 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> >> index 73e58a5108..bf14180f97 100644
> >> --- a/xen/arch/arm/irq.c
> >> +++ b/xen/arch/arm/irq.c
> >> @@ -23,6 +23,12 @@ const unsigned int nr_irqs = 
> >> IS_ENABLED(CONFIG_GICV3_ESPI) ?
> >>                                          (ESPI_MAX_INTID + 1) :
> >>                                          NR_IRQS;
> >>  
> >> +static bool irq_has_desc(unsigned int irq)
> > 
> > You are using this function only in one place, where you are actually
> > testing for SPI. So, maybe introduce irq_is_spi() helper instead? And
> > use it below?
> It can stay as is but:
>  - move it next to __irq_to_desc(),
>  - use it also as ASSERT(irq_has_desc(irq)) in __irq_to_desc() instead of the
> assertion you just added.
> This way the two stay in sync.

I will move irq_has_desc() next to __irq_to_desc() and add
ASSERT(irq_has_desc(irq)) at the start of __irq_to_desc().

> 
> > 
> >> +{
> >> +    return irq < NR_IRQS ||
> >> +           (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
> >> +}
> >> +
> >>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> >>  static DEFINE_SPINLOCK(local_irqs_type_lock);
> >>  
> >> @@ -76,7 +82,6 @@ static int __init init_espi_data(void)
> >>      return 0;
> >>  }
> >>  #else
> >> -
> > 
> > Please, no unnecessary changes
> > 
> >>  static int __init init_espi_data(void)
> >>  {
> >>      return 0;
> >> @@ -95,6 +100,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
> >>          return espi_to_desc(irq);
> >>  #endif
> >>  
> >> +    ASSERT(irq < NR_IRQS);
> check_timer_irq_cfg() in time.c calls irq_to_desc() on timer_irq[], and on the
> GTDT path those are not validated.

Patch 4 already handles this. It checks irq_set_type() before saving
each timer IRQ and stops boot if GTDT setup fails.

> 
> >> +
> >>      return &irq_desc[irq-NR_LOCAL_IRQS];
> >>  }
> >>  
> >> @@ -416,6 +423,9 @@ int setup_irq(unsigned int irq, unsigned int irqflags, 
> >> struct irqaction *new)
> >>      struct irq_desc *desc;
> >>      bool disabled;
> >>  
> >> +    if ( !gic_is_valid_line(irq) )
> Please add a printk message to inform the user.

Ack.

Best regards,
Mykola