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

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?

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5