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

Hi,

Mykola Kvach <[email protected]> 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 <[email protected]>
> ---
> 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?

> +{
> +    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);
> +
>      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) )
> +        return -EINVAL;
> +
>      desc = irq_to_desc(irq);
>  
>      spin_lock_irqsave(&desc->lock, flags);
> @@ -647,13 +657,21 @@ 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 )
> +    /*
> +     * This function should not be used for other than SPIs.
> +     *
> +     * 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) )

So here you can just call if ( !irq_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) )

-- 
WBR, Volodymyr