Re: [PATCH v2 2/4] KVM: arm64: vgic: Don't leak the SPI array when init is retried

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

Pre-existing issues:
- [High] If `vgic_init()` fails after `vgic_v4_init()` succeeds, the VPE array is left allocated but `vgic_initialized` remains false. Userspace can then create additional VCPUs and retry `vgic_init()`. The second `vgic_v4_init()` returns early because the VPE array is already allocated, leaving the new VCPUs without initialized VPE IRQs. When a new VCPU runs, `vgic_v4_load()` calls `irq_set_affinity()` with an uninitialized IRQ (0), which fails and triggers a host `WARN_ON`, causing a host-side denial of service.
--

--- Patch [2]: [PATCH v2 2/4] KVM: arm64: vgic: Don't leak the SPI array when init is retried ---
commit 11b9c8f9971b11e30ffc049a56071fafe4c7df73
Author: Fuad Tabba <[email protected]>

KVM: arm64: vgic: Don't leak the SPI array when init is retried

This patch fixes a memory leak where up to 960 struct vgic_irq could be leaked
if kvm_vgic_setup_default_irq_routing() fails and userspace retries
KVM_DEV_ARM_VGIC_CTRL_INIT. It resolves this by returning early from
kvm_vgic_dist_init() if dist->spis is already allocated.

> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 907057881b26a..d4cf143f3ae6b 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -210,6 +210,9 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  	struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);
>  	int i;
>  
> +	if (dist->spis)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but since the commit message mentions mirroring
the early return behavior of vgic_v4_init(), does that original function
have a stale state bug on retry?

If vgic_init() fails after vgic_v4_init() successfully allocates
dist->its_vm.vpes, vgic_initialized remains false. If userspace then creates
an additional vCPU and retries KVM_DEV_ARM_VGIC_CTRL_INIT, vgic_v4_init()
returns early on the second attempt without allocating a VPE for the newly
created vCPU.

When the new vCPU runs, vgic_v3_load() unconditionally calls vgic_v4_load().
Because the VPE was never properly initialized for this new vCPU,
vgic_v4_load() passes an uninitialized vpe->irq (0) to irq_set_affinity():

arch/arm64/kvm/vgic/vgic-v4.c:vgic_v4_load() {
        ...
        err = irq_set_affinity(vpe->irq, cpumask_of(smp_processor_id()));
        ...
}

This fails with -EINVAL and triggers a WARN_ON in vgic_v3_load():

arch/arm64/kvm/vgic/vgic-v3.c:vgic_v3_load() {
        ...
        WARN_ON(vgic_v4_load(vcpu));
        ...
}

Could an unprivileged VMM repeatedly trigger this host WARN_ON by
deliberately failing the first initialization and adding a vCPU before
retrying?

Is this something that should be addressed alongside or before this leak fix?

>  	dist->active_spis = (atomic_t)ATOMIC_INIT(0);
>  	dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);

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