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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Thu, Jul 16, 2026 at 12:20 PM Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>
>
>
> 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.

Thanks, I understand the intended split now.

I will call is_espi() unconditionally and add an espi_to_desc() stub for
the non-eSPI build, so the assertion inside is_espi() remains reachable.

I also agree that externally supplied IRQs should be rejected at fallible
entry points rather than relying on an assertion in __irq_to_desc().

The assertion in is_espi() only catches the configured-out eSPI range; it
does not cover the sparse 1024..4095 gap. Therefore, before dropping
ASSERT(irq < NR_IRQS), I will verify that every externally supplied path
which can reach irq_to_desc() rejects that gap. The remaining internal
callers should then satisfy the descriptor-backed IRQ invariant.

Best regards,
Mykola