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

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Fri, 2026-08-07 at 12:45 +0000, sashiko-bot@kernel.org wrote:
> 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 <Sascha.Bischoff@arm.com>
> 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.

Agreed, I think that this is an issue. The existing check only detects
vCPU creation that is already in progress. Since CTRL_INIT does not
hold kvm->lock, another creation can start afterwards and leave the
per-vCPU resources, including the doorbell domain, undersized. FWIW, I
think this affects GICv4, too.

I will post a separate fixup to serialise CTRL_INIT against vCPU
creation.

> 
> > +	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.

I've updated this commit to call vgic_v5_teardown() on subsequent
failure. This way no state is leaked.

> 
> 
> [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?

The current code matches what GICv4 does for the fwnode. In my
knowledge, it isn't a common thing for a single thread to create
multiple VMs. If this is indeed an issue, it can be revisited and
fixed, but the for the time being I suggest leaving this as is.

> 
> > +	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.

True. Dropped the unreachable code.

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

Thanks,
Sascha