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

On Fri, 2026-07-24 at 11:27 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [High] vgic_v5_send_command() unsafely passes 0 to
> irq_set_vcpu_affinity() if the doorbell IRQ is not allocated.
> --
> 
> --- Patch [14]: [PATCH v4 14/48] KVM: arm64: gic-v5: Set up VMTEs and
> VPE doorbells ---
> commit 3fe87417ec724cc6e6581baab62b9b85ad04ae50
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Set up VMTEs and VPE doorbells
> 
> This patch introduces the setup and teardown paths for GICv5 VM Table
> Entries
> and VPE doorbells. It allocates a VM ID, creates a per-VM VPE
> doorbell IRQ
> domain, and assigns doorbells to vCPUs for communication with the
> host IRS.
> Initialization and teardown workflows are implemented, including
> unwind paths
> for partial failures.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 826c2284a1453..4a62aa7d6505a 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> [ ... ]
> > @@ -406,6 +406,23 @@ static int vgic_v5_irs_set_up_vpe(u16 vm_id,
> > u16 vpe_id,
> >  	return 0;
> >  }
> >  
> > +static irqreturn_t db_handler(int irq, void *data)
> > +{
> > +	struct kvm_vcpu *vcpu = data;
> > +
> > +	WRITE_ONCE(vcpu->arch.vgic_cpu.vgic_v5.gicv5_vpe.db_fired,
> > true);
> > +
> > +	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> > +	kvm_vcpu_kick(vcpu);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int vgic_v5_send_command(struct kvm_vcpu *vcpu, enum
> > gicv5_vcpu_cmd cmd)
> > +{
> > +	return irq_set_vcpu_affinity(vgic_v5_vpe_db(vcpu), &cmd);
> > +}
> 
> [Severity: High]
> Does this code handle the case where the doorbell IRQ is not
> allocated yet or
> has already been freed, returning 0 from vgic_v5_vpe_db(vcpu)?
> 
> Passing 0 to irq_set_vcpu_affinity() can cause unexpected behavior,
> including
> potential memory corruption if IRQ 0 happens to be a valid interrupt
> on the
> platform.
> 
> There appear to be two paths where this happens. First,
> vgic_v5_reset() can be
> invoked before the VGIC is initialized, meaning the doorbell IRQ is
> still 0:
> 
> arch/arm64/kvm/vgic/vgic-v5.c:vgic_v5_reset()
> 	/* Make the VPE valid in the VPET */
> 	if (WARN_ON(vgic_v5_send_command(vcpu, VPE_MAKE_VALID)))
> 		return;

This is explicitly called from vgic_v5_init(), so that isn't a thing.

> 
> Second, a secondary teardown retry can trigger this in
> vgic_v5_teardown(). If
> an initial teardown failed to invalidate the VMTE, it leaves
> vmte_allocated = true but still frees the doorbells. A subsequent
> teardown
> will execute this with a 0 doorbell IRQ:
> 
> arch/arm64/kvm/vgic/vgic-v5.c:vgic_v5_teardown()
> 	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);

This one is a potential case where things could go wrong.

> 
> Could vgic_v5_send_command() verify the IRQ is non-zero before
> passing it
> to the IRQ core?
> 

I've added a small check in to ensure that the doorbell is non-zero
before proceeding.

Thanks,
Sascha