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

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

On 10-Jul-26 13:14, Mykola Kvach wrote:
> Hi Michal,
> 
> Thank you for the review.
> 
> On Fri, Jul 10, 2026 at 12:10:50PM +0200, 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.
> 
> I reproduced this on FVP by adding a fake DT interrupt with reserved
> INTID 3000. This was deliberately malformed fault injection. The only
> architecturally allocated interrupt class in the 1024-4095 gap is ePPI,
> which Xen does not currently support. The resulting out-of-bounds access
> to irq_desc[] may corrupt Xen memory or crash the hypervisor.
> 
> There is also a non-synthetic case with CONFIG_GICV3_ESPI=n. An
> architecturally valid eSPI described in DT reaches:
> 
>     platform_get_irq() -> irq_set_type() -> irq_set_spi_type()
> 
> Previously, irq_to_desc() was called before validation, while no
> espi_desc[] was compiled in. Moving the lookup after gic_is_spi() makes
> Xen reject the interrupt with -EINVAL instead.
It does not make much sense to me that is_espi() is protected in
__irq_to_desc(). I know this is because there is no espi_to_desc() if eSPI is
compiled out but providing a stub is easy. This causes the useful ASSERT inside
it to be unreachable. If is_espi() was meant to be called only under #ifdef, it
would not have the ASSERT and would not include #ifdef inside it. If we allowed
for that ASSERT, then ...

> 
> My criterion is to validate externally supplied IRQs at entry points
> which can return an error. The remaining callers use fixed,
> GIC-reported, or previously validated IRQs.
> 
> I will also add:
> 
>     ASSERT(irq < NR_IRQS);
... we would not need this one (the eSPI would also be more meaningful).

Given that we postponed the release, I'd be ok to take this series in,
provided it's in a correct shape.

~Michal