Re: [PATCH v5 12/49] KVM: arm64: gic-v5: Keep GICv5 vCPU limit model-specific

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] kvm_vgic_create() fails to validate that previously created VCPUs have IDs within the newly selected VGIC model's limits (for GICv2 and GICv3), leading to undefined behavior in GICv2 SGI routing.
--

--- Patch [12]: [PATCH v5 12/49] KVM: arm64: gic-v5: Keep GICv5 vCPU limit model-specific ---
commit 42bacf6540695fc12a35eee006d5382303176971
Author: Sascha Bischoff <[email protected]>

KVM: arm64: gic-v5: Keep GICv5 vCPU limit model-specific

This patch separates the IRS-derived vCPU limit for GICv5 from the generic
KVM maximum limit. It adds validation to reject GICv5 device creation if
existing VCPU IDs exceed the GICv5 model-specific limit.

> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 079a57c2b18f6..8e1e844ed6e93 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c

[ ... ]

> @@ -129,13 +129,34 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  	}
>  	ret = 0;
>  
> -	if (type == KVM_DEV_TYPE_ARM_VGIC_V2)
> +	switch (type) {
> +	case KVM_DEV_TYPE_ARM_VGIC_V2:
>  		kvm->max_vcpus = VGIC_V2_MAX_CPUS;
> -	else if (type == KVM_DEV_TYPE_ARM_VGIC_V3)
> +		break;
> +	case KVM_DEV_TYPE_ARM_VGIC_V3:
>  		kvm->max_vcpus = VGIC_V3_MAX_CPUS;
> -	else if (type == KVM_DEV_TYPE_ARM_VGIC_V5)
> -		kvm->max_vcpus = min(VGIC_V5_MAX_CPUS,
> -				     kvm_vgic_global_state.max_gic_vcpus);
> +		break;
> +	case KVM_DEV_TYPE_ARM_VGIC_V5:
> +		kvm->max_vcpus = kvm_vgic_global_state.max_gicv5_vcpus;
> +
> +		/*
> +		 * We use the userspace-allocated vcpu_id as the index into the
> +		 * GICv5 VPE table. This ensures that userspace's view and the
> +		 * guest's view of IAFFIDs remains consistent - GICv5's virtual
> +		 * IAFFID is the VPE ID in that VM's VPET.
> +		 *
> +		 * Reject vGICv5 creation if we have already got any vCPUs that
> +		 * we cannot back with a GICv5 VPE in the VPET.
> +		 */
> +		kvm_for_each_vcpu(i, vcpu, kvm) {
> +			if (vcpu->vcpu_id >= kvm->max_vcpus) {
> +				ret = -E2BIG;
> +				goto out_unlock;
> +			}
> +		}
> +
> +		break;
> +	}

[Severity: High]
This is a pre-existing issue, but does restricting this validation loop to 
only GICv5 leave the older models vulnerable to an out-of-bounds vcpu_id?

If userspace creates a VCPU with a large ID (for example, 500) before creating 
the VGIC device, it succeeds because the pre-VGIC limit defaults to the 
maximum supported by the system. If userspace then selects a GICv2 device, 
kvm_vgic_create() drops max_vcpus to 8. 

Because the vcpu_id check above is inside the GICv5 switch block, we skip 
validating the existing VCPUs against the new GICv2 limit.

When configuring private IRQs later during initialization, 
vgic_setup_private_irq() does this:

arch/arm64/kvm/vgic/vgic-init.c:vgic_setup_private_irq() {
    ...
    irq->targets = BIT(vcpu->vcpu_id);
    ...
}

Evaluating BIT(500) shifts a 64-bit integer by 500, which results in undefined
behavior in the kernel. A similar shift happens for SGI routing in 
vgic_mmio_write_sgir().

Should this validation loop be moved outside the switch statement so it runs 
for all VGIC models and prevents these out-of-bounds shifts across the board?

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