Re: [PATCH v5 08/49] KVM: arm64: gic-v5: Add VPE doorbell domain

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Resource leak of GICv5 VPE doorbell domain and fwnode on VGIC initialization retry.
- [High] TOCTOU race between `KVM_CREATE_VCPU` and `KVM_DEV_ARM_VGIC_CTRL_INIT` leads to undersized doorbell domain allocation.
- [Medium] Fwnode ID collision for VMs created by the same userspace thread.
- [Low] Unreachable dead code in doorbell domain error handling path.
--

--- Patch [8]: [PATCH v5 08/49] KVM: arm64: gic-v5: Add VPE doorbell domain ---
commit 087989feef3777beedf75a01512da5b9e331b6fa
Author: Sascha Bischoff <[email protected]>
KVM: arm64: gic-v5: Add VPE doorbell domain

This commit adds support for a per-VM VPE doorbell irq domain for GICv5.
It creates the domain under the GICv5 LPI domain with one doorbell allocated
per VPE, and stores the state for later IRS commands and wakeups.

diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
index 41ca2098ecb73..752329fc3d566 100644
--- a/arch/arm64/kvm/vgic/vgic-v5.c
+++ b/arch/arm64/kvm/vgic/vgic-v5.c
[ ... ]
> +static int vgic_v5_create_per_vm_domain(struct kvm *kvm)
> +{
> +	struct vgic_v5_vm *vm = &kvm->arch.vgic.gicv5_vm;
> +	int nr_vcpus = atomic_read(&kvm->online_vcpus);

[Severity: High]
Can a race condition here lead to an undersized doorbell domain allocation?

If a userspace thread calls the initialization ioctl, it checks for concurrent
VCPU creation, but this check is performed locklessly with respect to the
kvm->lock held during VCPU creation.

Concurrently, a second thread could create a VCPU, which takes kvm->lock and
increments created_vcpus. Since initialization hasn't completed yet, VCPU
creation is allowed to proceed.

When the first thread reaches this point, it allocates exactly the number
of online VCPUs at this moment. As the second thread completes and increments
online_vcpus, the VM now has more VCPUs than allocated doorbell IRQs, which
can lead to out-of-bounds IRQ array access later.

> +	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: High]
Can this leak the GICv5 VPE doorbell domain and fwnode if initialization
is retried?

If the initialization fails later in the setup process (for example, if
kvm_vgic_setup_default_irq_routing() fails in vgic_init()), the VGIC state
remains uninitialized.

If userspace then retries the initialization, this function is called again
and unconditionally allocates a new fwnode and IRQ domain, overwriting
the previous pointers without freeing them.


[Severity: Medium]
Could using the thread ID here cause fwnode naming collisions?

Since task_pid_nr(current) is used as the ID for the fwnode name, a single
VMM thread sequentially creating multiple KVM VMs will cause them to share
the exact same fwnode name.

When the IRQ domain is registered, this duplicate name causes debugfs
directory creation failures due to the directory already being present.

Would it be more reliable to use the VM's unique vm_id instead?

> +	if (!vm->fwnode)
> +		return -ENOMEM;
[ ... ]
> +	db_virq = irq_domain_alloc_irqs(vm->domain, nr_vcpus, NUMA_NO_NODE, vm);
> +	if (db_virq <= 0) {
> +		ret = db_virq;
> +		goto err;
> +	}
> +
> +	kvm->arch.vgic.gicv5_vm.vpe_db_base = db_virq;
> +
> +	return 0;
> +
> +err:
> +	if (db_virq > 0)

[Severity: Low]
Is this error cleanup path unreachable?

The only branch that jumps to the err label after modifying db_virq
explicitly checks that db_virq is less than or equal to 0.

Because db_virq can never be strictly positive at this label, the
irq_domain_free_irqs() block will never be executed.

> +		irq_domain_free_irqs(db_virq, nr_vcpus);
> +	if (vm->domain)
> +		irq_domain_remove(vm->domain);
[ ... ]

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