Re: [PATCH for-4.22 1/2] xen/arm: validate IRQs before descriptor lookup

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

On 10-Jul-26 10:31, Mykola Kvach wrote:
> With GICv3 eSPI support, nr_irqs covers the architectural INTID
> namespace up to ESPI_MAX_INTID. That namespace is not backed by a
> single dense irq_desc[] array: regular SPIs use irq_desc[], eSPIs use
> espi_desc[], and the INTIDs in between must not be treated as
> descriptor indices.
> 
> A firmware-provided interrupt in the sparse range can therefore pass
> checks based only on nr_irqs and reach irq_to_desc().
What are the possible consequences? You should explain it in the commit msg.
Also, what is your criteria behind protecting irq_to_desc()? There are other
places, where we have unprotected irq_to_desc(). Last but not least I think we
should have an ASSERT in __irq_to_desc() to prevent the indexing failure if we
end up there with a sparse IRQ accidentally.

The change itself looks ok.

~Michal
> 
> Validate IRQs with the GIC line helpers before looking up descriptors
> in setup_irq(). Also validate SPI type configuration with gic_is_spi()
> before irq_set_spi_type() obtains the descriptor, covering the
> platform_get_irq() -> irq_set_type() path. The route-to-Xen assert
> becomes redundant because setup_irq() now rejects invalid IRQs before
> routing.
> 
> Fixes: 98f7060b9ed ("xen/arm/irq: add handling for IRQs in the eSPI range")
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
>  xen/arch/arm/gic.c | 2 --
>  xen/arch/arm/irq.c | 9 +++++++--
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ee75258fc3..ed9870c1bd 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -111,8 +111,6 @@ static void gic_set_irq_priority(struct irq_desc *desc, 
> unsigned int priority)
>  void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
>  {
>      ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
> -    /* Can't route interrupts that don't exist */
> -    ASSERT(gic_is_valid_line(desc->irq));
>      ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>      ASSERT(spin_is_locked(&desc->lock));
>  
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 73e58a5108..0314cf0b6e 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -416,6 +416,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 +650,15 @@ 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 */
> -    if ( spi < NR_LOCAL_IRQS )
> +    if ( !gic_is_spi(spi) )
>          return -EINVAL;
>  
> +    desc = irq_to_desc(spi);
> +
>      spin_lock_irqsave(&desc->lock, flags);
>  
>      if ( !irq_validate_new_type(desc->arch.type, type) )