Re: [PATCH v4 27/48] KVM: arm64: gic-v5: Support SPI injection

On Fri, 2026-07-24 at 11:48 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
> 
> New issues:
> - [Critical] `vgic_v5_set_irq_pend` dereferences `vcpu->kvm` without
> checking for NULL, causing a host kernel panic during SPI or LPI
> injection.
> - [High] GICv5 SPIs silently break `KVM_IRQFD_FLAG_RESAMPLE` because
> KVM bypasses AP list queueing and never observes guest EOIs.
> - [High] GICv5 SPI injection is broken for generic KVM callers
> because `vgic_get_irq` expects a GICv5-encoded INTID, but external
> callers pass raw INTIDs.
> 
> Pre-existing issues:
> - [High] Pre-existing Spectre v1 vulnerability in `vgic_get_irq()`
> when handling GICv2/GICv3 SPIs. Speculative execution with `intid <
> VGIC_NR_PRIVATE_IRQS` bypasses bounds checks and leads to negative
> indexing on the `spis` array.
> - [High] Invalid GICv5 PPI IDs silently alias to PPI 0 (SGI 0) in
> `vgic_get_vcpu_irq()` due to a missing architectural bounds check.
> --
> 
> --- Patch [27]: [PATCH v4 27/48] KVM: arm64: gic-v5: Support SPI
> injection ---
> commit 416f67edd52d537473f18e3ac4c5b905a4c03f05
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Support SPI injection
> 
> GICv5 SPI lifecycle is handled by the GICv5 hardware once the pending
> state has been injected. This change adds support for injecting SPIs
> to
> the core VGIC code and GICv5 code.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 7957b9da95e7c..04392b686c2e2 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> > @@ -1064,6 +1064,47 @@ void vgic_v5_flush_ppi_state(struct kvm_vcpu
> > *vcpu)
> 
> [ ... ]
> 
> > +static bool vgic_v5_spi_queue_irq_unlock(struct kvm *kvm,
> > +					 struct vgic_irq *irq,
> > +					 unsigned long flags)
> > +	__releases(&irq->irq_lock)
> > +{
> > +	lockdep_assert_held(&irq->irq_lock);
> > +
> > +	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> > +	return true;
> > +}
> 
> [Severity: High]
> Does skipping the AP list queueing here break
> KVM_IRQFD_FLAG_RESAMPLE?
> 
> Since this function overrides the default queueing logic and prevents
> the SPI
> from being added to the vCPU's AP list, the lifecycle becomes fully
> hardware
> managed.

Correct. That's exactly what it is doing. GICv5 manages the interrupt
lifecycle in hardware as much as possible.

> 
> Because KVM won't fold the SPI state back, it seems we'll never
> observe guest
> EOIs, meaning kvm_notify_acked_irq() won't be called to trigger the
> resample
> eventfd.

Indeed. We've dropped this support for the time being. It was present
in a previous version of this series, but is no longer present.

> 
> Could this lead to silently hanging devices if userspace (like QEMU
> virtio-mmio
> or VFIO) attaches an eventfd using the resample flag?
> 
> [ ... ]
> 
> > +/* Set the pending state for GICv5 SPIs and LPIs */
> > +void vgic_v5_set_irq_pend(struct kvm_vcpu *vcpu, struct vgic_irq
> > *irq)
> > +{
> > +	if (WARN_ON(__irq_is_ppi(KVM_DEV_TYPE_ARM_VGIC_V5, irq-
> > >intid)))
> > +		return;
> > +
> > +	kvm_call_hyp(__vgic_v5_vdpend, irq->intid,
> > irq_is_pending(irq),
> > +		     vcpu->kvm->arch.vgic.gicv5_vm.vm_id);
> > +}
> 
> [Severity: Critical]
> Is it possible for vcpu to be NULL here?

We should not be able to reach this with a NULL here.

> 
> If userspace configures the IRS and injects an SPI or unmapped LPI
> (e.g., via
> KVM_IRQ_LINE) before any vCPUs are created, irq->target_vcpu could be
> NULL.

No. LPIs are not injected that way anyhow. For SPIs, we explicitly
initialise those with target_vcpu = vcpu0.

> 
> This would unconditionally dereference vcpu->kvm, potentially causing
> a host
> kernel panic.
> 
> Should we add a NULL check for vcpu before dereferencing it?
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic.c
> > b/arch/arm64/kvm/vgic/vgic.c
> > index 023792c28fc5c..d571fafb07f32 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)) {
> 
> [Severity: High]
> Does this break SPI injection for generic KVM callers on GICv5?
> 
> The __irq_is_spi() helper expects a GICv5-encoded INTID when type is
> KVM_DEV_TYPE_ARM_VGIC_V5.

Correct. That's very much the point.

> 
> However, standard in-kernel device emulations (like the PMU) and
> userspace
> injections via KVM_IRQ_LINE or irqfd pass raw integer INTIDs (e.g.,
> 32).

PMU requires a PPI on GICv5. KVM_IRQ_LINE builds a GICv5 SPI explicitly
for a GICv5 VM.

> 
> Without the GICv5 type bits, __irq_is_spi() will return false,
> causing the
> injection to abort with -EINVAL.

As it should, but that's not the case for anything that provides the
correct ID.

> 
> > +		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;
> > +
> > +			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 does this speculative bounds
> checking logic
> have a Spectre v1 vulnerability?
> 
> The array_index_nospec() clamp is applied to intid before subtracting
> VGIC_NR_PRIVATE_IRQS (32).
> 
> If intid is speculatively less than 32, the check (intid < max_intid)
> will
> speculatively evaluate to true, and array_index_nospec will preserve
> the
> value.
> 
> Subtracting 32 then results in a negative index, allowing speculative
> out-of-bounds reads on the spis array.

No. With GICv2/3 SPIs start at 32. This is adding 32 to the number of
SPIs, checking that the ID is within that range, then subtracting the
32 again to index into the array of SPIs.

> 
> > +		}}
> >  	}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but looking at
> vgic_get_vcpu_irq()
> just below this function, it appears there is a missing bounds check
> for GICv5
> PPI IDs:
> 
> struct vgic_irq *vgic_get_vcpu_irq(struct kvm_vcpu *vcpu, u32 intid)
> {
>     ...
>     if (__irq_is_sgi(type, intid) || __irq_is_ppi(type, intid)) {
>         switch (type) {
>         case KVM_DEV_TYPE_ARM_VGIC_V5:
>             intid = vgic_v5_get_hwirq_id(intid);
>             intid = array_index_nospec(intid,
> VGIC_V5_NR_PRIVATE_IRQS);
>             break;
> 
> Unlike the SPI path, this extracts the hardware ID and directly
> applies
> array_index_nospec() without an explicit bounds check (e.g. checking
> if
> intid >= VGIC_V5_NR_PRIVATE_IRQS).

But the array_index_nospec() is doing that capping...

> 
> Since __irq_is_ppi() only checks the GICv5 type bits and not the ID
> range, an
> invalid out-of-bounds PPI ID will silently alias to index 0.

Yes, it would. That is the point.

> 
> Can this lead to silent corruption of SGI 0 state?
> 
No, but it would cause that to happen to PPI 0, but that's fine.
Garbage in, garbage out.

Thanks,
Sascha