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

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

On 25-Aug-26 02:30, Volodymyr Babchuk wrote:
> Hi,
> 
> Mykola Kvach <mykola_kvach@xxxxxxxx> 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 <mykola_kvach@xxxxxxxx>
>> ---
>> 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?
It can stay as is but:
 - move it next to __irq_to_desc(),
 - use it also as ASSERT(irq_has_desc(irq)) in __irq_to_desc() instead of the
assertion you just added.
This way the two stay in sync.

> 
>> +{
>> +    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);
check_timer_irq_cfg() in time.c calls irq_to_desc() on timer_irq[], and on the
GTDT path those are not validated.

>> +
>>      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) )
Please add a printk message to inform the user.

>> +        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) )
> 

~Michal