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

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

On a tangent note:
I can see that you pushed quite a few "for-4.22" patches. We are approaching the
release, so afaict at this point we should only be taking crucial bug fixes.
Moreover, when sending "for-X" patches, please include a description with your
analyzed pros/cons of taking a patch in.

~Michal

On 10-Jul-26 12:10, Orzel, Michal wrote:
> 
> 
> 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) )
> 
>