Re: [PATCH v2 1/3] xen/arm: validate IRQs before descriptor lookup

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

Hi Michal,

Thank you for the review.

On Tue, Aug 11, 2026 at 10:51:47AM +0200, Orzel, Michal wrote:
> 
> 
> On 10-Aug-26 20:38, Mykola Kvach wrote:
> > 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.
> > 
> > Call is_espi() unconditionally in __irq_to_desc() and provide an
> > espi_to_desc() stub when eSPI support is disabled. This preserves the
> > is_espi() debug check for eSPI-range INTIDs when support is disabled.
> > 
> > Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > 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 | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 73e58a5108..0f5d3496bf 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)
> > +{
> > +    return irq < NR_IRQS ||
> > +           (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
> This IS_ENABLED reads redundant because is_espi() contains #ifdef
> CONFIG_GICV3_ESPI inside. AFAICT you added it here to prevent the !ESPI build
> from reaching ASSERT inside is_espi() when the irq is in ESPI range. I don't
> like the ASSERT inside is_espi(). I think it does not make much sense in a
> helper that should really just tell us whether the IRQ is in ESPI range or 
> not.
> It should be up to the caller to decide what to do based on whether ESPI is
> compiled in or not. I think this cleanup would be best to be done first. If 
> you
> don't want to do that, at least document this in the commit msg because others
> may be tempted to drop this IS_ENABLED.

Ack. I’ll add a preparatory cleanup patch making is_espi() a pure
range predicate and keep the configuration handling at the call sites.

> 
> > +}
> > +
> >  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> >  static DEFINE_SPINLOCK(local_irqs_type_lock);
> >  
> > @@ -77,6 +83,12 @@ static int __init init_espi_data(void)
> >  }
> >  #else
> >  
> > +static struct irq_desc *espi_to_desc(unsigned int irq)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return NULL;
> > +}
> > +
> >  static int __init init_espi_data(void)
> >  {
> >      return 0;
> > @@ -90,10 +102,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
> >      if ( irq < NR_LOCAL_IRQS )
> >          return &this_cpu(local_irq_desc)[irq];
> >  
> > -#ifdef CONFIG_GICV3_ESPI
> >      if ( is_espi(irq) )
> >          return espi_to_desc(irq);
> > -#endif
> >  
> >      return &irq_desc[irq-NR_LOCAL_IRQS];
> Nothing here covers 1024..4095. I think we should add at least:
> ASSERT(irq < NR_IRQS) like we discussed some time ago.

Ack, I’ll restore the assertion before indexing irq_desc[].

> 
> >  }
> > @@ -416,6 +426,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) )
> > +        return -EINVAL;
> > +
> >      desc = irq_to_desc(irq);
> >  
> >      spin_lock_irqsave(&desc->lock, flags);
> > @@ -647,13 +660,19 @@ static bool irq_validate_new_type(unsigned int curr, 
> > unsigned int new)
> >  int irq_set_spi_type(unsigned int spi, unsigned int type)
> >  {
> >      unsigned long flags;
> > -    struct irq_desc *desc = irq_to_desc(spi);
> > +    struct irq_desc *desc;
> >      int ret = -EBUSY;
> >  
> > -    /* This function should not be used for other than SPIs */
> This is an important line that you should keep.

Ack.

Best regards,
Mykola