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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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.

> +}
> +
>  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.

>  }
> @@ -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.

> -    if ( spi < NR_LOCAL_IRQS )
> +    /*
> +     * The implemented GIC line counts are not available when early
> +     * callers configure IRQ types. Check descriptor storage here; 
> setup_irq()
> +     * validates the implemented line before the interrupt is used.
> +     */
> +    if ( spi < NR_LOCAL_IRQS || !irq_has_desc(spi) )
>          return -EINVAL;
>  
> +    desc = irq_to_desc(spi);
> +
>      spin_lock_irqsave(&desc->lock, flags);
>  
>      if ( !irq_validate_new_type(desc->arch.type, type) )

~Michal