Re: [PATCH v5 14/49] KVM: arm64: gic-v5: Set up VMTEs and VPE doorbells

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double-teardown vulnerability allows corrupted VMTE slots to be reused, subverting the explicit VM ID leak mechanism.
- [Medium] vgic_v5_create_per_vm_domain uses task_pid_nr(current) for the fwnode name, causing debugfs collisions for multiple VMs in the same process.
--

--- Patch [14]: [PATCH v5 14/49] KVM: arm64: gic-v5: Set up VMTEs and VPE doorbells ---
commit 7e8599087432413da91f2b37e503a3ba7e749684
Author: Sascha Bischoff <[email protected]>

KVM: arm64: gic-v5: Set up VMTEs and VPE doorbells

A GICv5 VM needs a VM table entry before it can use SPIs and LPIs,
which are backed by the host IRS. The VM table itself is created at
probe time, but each VM still needs to claim and populate one VMTE
before it can use those interrupts. Allocate a VM ID during
vgic_v5_init() and create a per-VM VPE doorbell irq domain.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> index 2121f67d3b59c..f15a8309d7a93 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
[ ... ]
> @@ -491,20 +524,10 @@ static int vgic_v5_create_per_vm_domain(struct kvm *kvm)
>  	int id = task_pid_nr(current);
>  	int ret, db_virq = 0;
>  
> -	if (!gicv5_global_data.lpi_domain) {
> -		kvm_err("LPI domain uninitialized, can't set up KVM Doorbells\n");
> -		return -ENODEV;
> -	}
> -
>  	vm->fwnode = irq_domain_alloc_named_id_fwnode("GICv5-vpe-db", id);

[Severity: Medium]
Does using task_pid_nr() here cause debugfs collisions if a single process
creates multiple VMs?

If userspace instantiates multiple GICv5 VMs within the same process, they
will share the same PID. The irq_domain_alloc_named_id_fwnode() call would
then create a fwnode based on the PID instead of a unique identifier like
the VM ID.

Destroying the first VM removes the shared directory, breaking it for the
remaining VMs.

[ ... ]
> +void vgic_v5_teardown(struct kvm *kvm)
> +{
> +	struct vgic_dist *dist = &kvm->arch.vgic;
> +	struct kvm_vcpu *vcpu, *vcpu0;
> +	bool release_vm_id = true;
> +	unsigned long i;
> +	int rc;
> +
> +	lockdep_assert_held(&kvm->arch.config_lock);
> +
> +	/*
> +	 * If the VM's ID isn't valid, then we either failed init very early or
> +	 * we've been called a second time. Nothing to do here in either case.
> +	 */
> +	if (kvm->arch.vgic.gicv5_vm.vm_id == VGIC_V5_VM_ID_INVAL)
> +		return;
> +
> +	if (kvm->arch.vgic.gicv5_vm.vmte_allocated) {
> +		/* Make the VM invalid  */
> +		vcpu0 = kvm_get_vcpu(kvm, 0);
> +		rc = vgic_v5_send_command(vcpu0, VMTE_MAKE_INVALID);
> +		if (rc) {
> +			kvm_err("could not make VMTE invalid\n");
> +			release_vm_id = false;
> +			goto out_free_doorbells;
> +		}
> +
> +		kvm_for_each_vcpu(i, vcpu, kvm) {
> +			if (vgic_v5_vmte_free_vpe(vcpu)) {
> +				kvm_err("Failed to free VPE\n");
> +				release_vm_id = false;
> +			}
> +		}
> +
> +		if (vgic_v5_vmte_release(kvm)) {
> +			kvm_err("Failed to release VM 0x%x\n", dist->gicv5_vm.vm_id);
> +			release_vm_id = false;
> +		}
> +	}

[Severity: High]
Could a double-teardown incorrectly release the VM ID despite a previous
failure here, allowing corrupted VMTE slots to be reused?

During the first run of vgic_v5_teardown(), if vgic_v5_vmte_free_vpe() fails,
release_vm_id is set to false to leak the VM ID and prevent reuse. However,
if vgic_v5_vmte_release() then succeeds, it will clear vmte_allocated.

> +
> +out_free_doorbells:
> +	vgic_v5_free_doorbells(kvm, atomic_read(&kvm->online_vcpus));
> +	vgic_v5_teardown_per_vm_domain(&kvm->arch.vgic.gicv5_vm);
> +
> +	/*
> +	 * We only release the VM ID itself if we didn't fail earlier. It does
> +	 * mean that we might lose the VM ID (and associated VMTE, etc), but
> +	 * given that we've failed to tear them down correctly there's no way to
> +	 * safely reuse them. The VM ID allocating IDA will make sure we don't
> +	 * accidentally reuse this partially torn down state.
> +	 */
> +	if (release_vm_id)
> +		vgic_v5_release_vm_id(kvm);
> +}

When vgic_v5_teardown() is called a second time (e.g., on VM destruction
after an initial failure), vm_id is still valid, but vmte_allocated is now
false. This causes it to bypass the block that sets release_vm_id to false
on errors.

Because release_vm_id defaults to true at the top of the function, it then
proceeds to unconditionally release the VM ID, subverting the safety
mechanism and potentially leading to hardware state corruption.

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