Re: [PATCH 5/5] KVM: arm64: vgic: Reject out-of-range GICv5 PPI IDs

On Mon, 2026-08-10 at 11:16 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Critical] Returning NULL for out-of-range GICv5 PPIs causes a
> deterministic host kernel panic via NULL pointer dereference or
> BUG_ON() during timer initialization with user-controlled inputs.
> --
> 
> --- Patch [5]: [PATCH 5/5] KVM: arm64: vgic: Reject out-of-range
> GICv5 PPI IDs ---
> commit b745364de86ed8f379d9297c2d334e7a734654d6
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: vgic: Reject out-of-range GICv5 PPI IDs
> 
> The commit prevents KVM from clamping out-of-range GICv5 PPIs to
> index 0.
> It introduces a check in vgic_get_vcpu_irq() that returns NULL when
> the
> extracted hardware IRQ ID exceeds the supported range.
> 
> Link:
> https://sashiko.dev/#/patchset/[email protected]?part=27
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic.c
> > b/arch/arm64/kvm/vgic/vgic.c
> > index 3077cfdaa146d..d0c91f0c52698 100644
> > --- a/arch/arm64/kvm/vgic/vgic.c
> > +++ b/arch/arm64/kvm/vgic/vgic.c
> > @@ -118,6 +118,8 @@ struct vgic_irq *vgic_get_vcpu_irq(struct
> > kvm_vcpu *vcpu, u32 intid)
> >  		switch (type) {
> >  		case KVM_DEV_TYPE_ARM_VGIC_V5:
> >  			intid = vgic_v5_get_hwirq_id(intid);
> > +			if (intid >= VGIC_V5_NR_PRIVATE_IRQS)
> > +				return NULL;
> 
> [Severity: Critical]
> Does returning NULL here allow a malicious userspace process to
> trigger a host
> kernel panic?

We already return NULL on the other paths so this is consistent
behaviour. However, the following is definitely an issue, albeit not
with the generic vgic code.

> 
> If a user configures a VM and sets the timer IRQ to an out-of-bounds
> GICv5 PPI
> ID (>= 64) via the KVM_SET_DEVICE_ATTR ioctl, the input is validated
> using
> irq_is_ppi(). Because irq_is_ppi() only checks the upper type bits
> for GICv5,
> it accepts the invalid ID and stores it.
> 
> When the vCPU is subsequently run, kvm_timer_enable() initializes the
> timers
> and calls functions like kvm_vgic_set_owner() and
> kvm_vgic_map_phys_irq() with
> the out-of-bounds IRQ. These callers lack NULL checks and assume the
> returned
> pointer is valid.
> 
> For example, kvm_vgic_set_owner() unconditionally dereferences it:
> 
> arch/arm64/kvm/vgic/vgic.c:kvm_vgic_set_owner() {
> 	irq = vgic_get_vcpu_irq(vcpu, intid);
> 	raw_spin_lock_irqsave(&irq->irq_lock, flags);
> 	...
> }
> 
> And kvm_vgic_map_phys_irq() explicitly asserts it with BUG_ON():
> 
> arch/arm64/kvm/vgic/vgic.c:kvm_vgic_map_phys_irq() {
> 	struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, vintid);
> 	...
> 	BUG_ON(!irq);
> 	...
> }
> 
> Should the boundary check be enforced earlier, such as during the
> ioctl
> validation itself, to prevent storing the invalid ID and crashing
> during
> timer initialization?

We should fix both issues, and shouldn't work around an arch timer
issue in the common vgic code.

I've added a check to the irq_is_ppi() helper that check that the ID is
valid for GICv5. This is safer and more consistent with what was done
for GICv2/3.

I've had a closer look at this and can confirm that the arch timer code
has a bug with GICv5 where we look up a potentially invalid IRQ, then
pass the NULL pointer to kvm_vgic_set_owner(), which then tries to
dereference it.  I'd added another patch to v2 of this series to first
check the PPI is correct and only then try and set the owner.

> 
> >  			intid = array_index_nospec(intid,
> > VGIC_V5_NR_PRIVATE_IRQS);
> >  			break;
> >  		default:
> 

Thanks,
Sascha