Re: [PATCH v3 29/40] KVM: arm64: gic-v5: Support SPI injection

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Fri, 2026-07-03 at 16:53 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> New issues:
> - [High] O(N^2) hypercalls inside a global raw spinlock on the guest
> exit path.
> - [High] VFIO level-triggered SPIs are permanently lost because EOI
> notifications are gated behind `!irq_is_pending()`.
> - [Medium] Guest-triggerable host dmesg spam via `nr_spis` clamping
> mismatch in IRS capability registers.
> 
> Pre-existing issues:
> - [High] Spectre v1 gadget in `vgic_get_irq()` for GICv2/v3 SPIs due
> to subtraction after `array_index_nospec()`.
> --
> 
> commit 4f4d623ce71f96a23431e620fddd39282b0000ad
> Author: Sascha Bischoff <Sascha.Bischoff@arm.com>
> Subject: KVM: arm64: gic-v5: Support SPI injection
> 
> This commit adds support for injecting and managing GICv5 SPIs in the
> VGIC
> core. It introduces SPI lookup, irq_ops for state management, and a
> global
> AP list for tracking SPIs during VCPU exits.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 20ae7daf4c451..ceab71c2412bd 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> > @@ -1113,6 +1113,99 @@ void vgic_v5_fold_irq_state(struct kvm_vcpu
> > *vcpu)
> 
> [Severity: High]
> Will this loop iteration cause soft lockups and VCPU starvation?
> 
> Looking at vgic_v5_fold_irq_state(), the host iterates over the
> global
> vgic_v5_spi_ap_list_head while holding a raw spinlock and making
> hypercalls:
> 
> arch/arm64/kvm/vgic/vgic-v5.c:vgic_v5_fold_irq_state() {
> retry:
>     raw_spin_lock(&vgic_dist->vgic_v5_spi_ap_list_lock);
>     list_for_each_entry(irq, &vgic_dist->vgic_v5_spi_ap_list_head,
> ap_list) {
>         ...
>         icsr = kvm_call_hyp_ret(__vgic_v5_vdrcfg, irq->intid);
>         ...
>         if (!irq->active && !pending && !irq_is_pending(irq)) {
>             ...
>             list_del(&irq->ap_list);
>             ...
>             raw_spin_unlock(&vgic_dist->vgic_v5_spi_ap_list_lock);
>             ...
>             goto retry;
>         }
>     }
> }
> 
> If multiple SPIs are deactivated simultaneously, dropping the locks
> and
> restarting the iteration from the beginning of the list requires
> repeatedly
> evaluating already visited active SPIs, resulting in O(N^2)
> hypercalls
> inside a raw spinlock.

This has been discussed in a previous review on this series (patch
#27). In summary, yes, it would, but I am dropping this part of the
support for now.

> 
> [Severity: High]
> Does this condition permanently lose VFIO level-triggered SPIs?
> 
> In the same loop in vgic_v5_fold_irq_state(), the End Of Interrupt
> (EOI)
> notification is gated behind a check that irq_is_pending() is false:
> 
> arch/arm64/kvm/vgic/vgic-v5.c:vgic_v5_fold_irq_state() {
>         ...
>         if (!irq->active && !pending && !irq_is_pending(irq)) {
>             ...
>             kvm_notify_acked_irq(vcpu->kvm, 0, intid);
>         ...
> }
> 
> For level-triggered interrupts injected by VFIO via irqfd, KVM sets
> irq->line_level = 1. The irq_is_pending() function checks this
> line_level
> and returns true, which prevents the EOI notification from being
> sent.
> 
> Without the notification, the irqfd resampler never deasserts the
> interrupt
> and clears line_level, leaving the interrupt permanently blocked.

Again, discussed previously. This part of the changeset is being
dropped.

> 
> >  	raw_spin_unlock(&vgic_dist->vgic_v5_spi_ap_list_lock);
> >  }
> >  
> > +static bool vgic_v5_set_spi_pending_state(struct kvm_vcpu *vcpu,
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic.c
> > b/arch/arm64/kvm/vgic/vgic.c
> > index ee7fc0c7104e6..26f9ff7700429 100644
> > --- a/arch/arm64/kvm/vgic/vgic.c
> > +++ b/arch/arm64/kvm/vgic/vgic.c
> > @@ -86,19 +86,31 @@ static struct vgic_irq *vgic_get_lpi(struct kvm
> > *kvm, u32 intid)
> >   */
> >  struct vgic_irq *vgic_get_irq(struct kvm *kvm, u32 intid)
> >  {
> > -	/* Non-private IRQs are not yet implemented for GICv5 */
> > -	if (vgic_is_v5(kvm))
> > -		return NULL;
> > +	enum kvm_device_type type = kvm->arch.vgic.vgic_model;
> >  
> >  	/* SPIs */
> > -	if (intid >= VGIC_NR_PRIVATE_IRQS &&
> > -	    intid < (kvm->arch.vgic.nr_spis +
> > VGIC_NR_PRIVATE_IRQS)) {
> > -		intid = array_index_nospec(intid, kvm-
> > >arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS);
> > -		return &kvm->arch.vgic.spis[intid -
> > VGIC_NR_PRIVATE_IRQS];
> > +	if (__irq_is_spi(type, intid)) {
> > +		switch (type) {
> > +		case KVM_DEV_TYPE_ARM_VGIC_V5:
> > +			intid = vgic_v5_get_hwirq_id(intid);
> > +
> > +			if (intid >= kvm->arch.vgic.nr_spis)
> > +				return NULL;
> 
> [Severity: Medium]
> Is it possible for a guest to spam the host kernel log by exploiting
> a
> mismatch in the nr_spis values?
> 
> In kvm_vgic_v5_irs_init(), the local nr_spis variable is clamped up
> to
> VGIC_V5_DEFAULT_NR_SPIS (32), but the global kvm->arch.vgic.nr_spis
> is left
> unchanged (e.g., at 16).
> 
> This clamped value is exposed to the guest in the IRS IDR registers.
> If the
> guest writes an ID between 16 and 31 to IRS_SPI_SELR, it is accepted
> because
> vgic_v5_is_spi_selr_valid() checks against the clamped value.
> 
> When vgic_v5_mmio_read_irs_spi() attempts to read it, this check
> fails and
> returns NULL, which then triggers an unratelimited kvm_err():
> 
> arch/arm64/kvm/vgic/vgic-irs-v5.c:vgic_v5_mmio_read_irs_spi() {
>     ...
>     if (!irq) {
>         kvm_err("Guest trying to access SPI not backed by KVM\n");
>     ...
> }
> 

We only allow userspace to set multiples of 32, and always mandate that
SPIs are supported by setting the minimum value to 32. If we ever end
up in the situation where we're trying to access an SPI that isn't
backed, we absolutely want to catch that, so this return of NULL is
very intentional.

The guest is able to write any value it wishes to the IRS_SPI_SELR, so
we actually rely on the NULL to make sure that the guest is accessing
an SPI it is allowed to.

I've already demoted the kvm_err() to a kvm_debug() as part of one of
the previous code reviews, so this will only "spam" the log if
debugging is enabled in the first place.

> > +
> > +			intid = array_index_nospec(intid, kvm-
> > >arch.vgic.nr_spis);
> > +			return &kvm->arch.vgic.spis[intid];
> > +		default: {
> > +			u32 max_intid = kvm->arch.vgic.nr_spis +
> > VGIC_NR_PRIVATE_IRQS;
> > +
> > +			if (intid < max_intid) {
> > +				intid = array_index_nospec(intid,
> > max_intid);
> > +				return &kvm->arch.vgic.spis[intid
> > - VGIC_NR_PRIVATE_IRQS];
> 
> [Severity: High]
> This is a pre-existing issue, but could this allow an out-of-bounds
> access
> under speculative execution?
> 
> If an attacker can control intid and speculatively bypass the
> __irq_is_spi() bounds check, array_index_nospec() bounds it against
> the upper
> limit. However, a value like 0 subtracts VGIC_NR_PRIVATE_IRQS (32)
> and
> creates a large unsigned underflow, which may allow accessing out of
> bounds
> memory.
> 

Actually, we only call this code if we detect the incoming interrupt ID
as being an SPI. For vGICv2/3 we explicitly check that the value is
above the number of private IRQs too. Hence, we never even enter this
code block unless we're already sure that it is the possible range of
SPIs. Then, this code imposes an upper limited based on the specific
VM's configuration.

In summary, I think this is fine.

> > +			}
> > +		}}
> >  	}
> >  
> >  	/* LPIs */
> 

Thanks,
Sascha