Re: [PATCH v4 35/48] KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

On Fri, 2026-07-24 at 12:02 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential
> issue(s) to consider:
> - [Critical] Retrieving `vcpu0` without verifying it exists leads to
> a NULL pointer dereference.
> - [Critical] Inverted lock check with `kvm_trylock_all_vcpus` leaks
> vCPU locks and runs lockless.
> - [High] Incomplete cleanup on IST restore failure leaves a dangling
> pointer in `vmi->h_lpi_ist`, leading to a double free.
> - [High] Missing validation of LPI INTIDs during IST restore allows
> userspace to inject invalid INTIDs.
> - [High] SPI IST restore incorrectly uses the array index directly as
> the absolute INTID, causing wrong interrupts to be injected.
> - [High] Quiescence check masks lost quiescence state and causes
> silent data corruption.
> - [Medium] KVM_DEV_ARM_VGIC_GRP_IST unconditionally fails for VMs
> configured with 0 SPIs, breaking live migration.
> - [Medium] The failure path in `vgic_v5_irs_restore_ists` incorrectly
> issues a redundant `VMTE_MAKE_INVALID` command, risking a memory
> leak.
> --
> 
> --- Patch [35]: [PATCH v4 35/48] KVM: arm64: gic-v5: Implement
> save/restore mechanisms for ISTs ---
> commit 5c47453f1884c74d1478029039189ee4ba68421a
> Author: Sascha Bischoff <[email protected]>
> Subject: KVM: arm64: gic-v5: Implement save/restore mechanisms for
> ISTs
> 
> This patch introduces save and restore mechanisms for GICv5 Interrupt
> State
> Tables (ISTs). It provides userspace migration storage for SPI and
> LPI
> buffers via KVM_DEV_ARM_VGIC_GRP_IST and ensures the VM is quiesced
> and
> validated during operations.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > index 718410e8c9f3b..f220267266c29 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> 
> [ ... ]
> 
> > @@ -1630,6 +1630,28 @@ static int vgic_v5_restore_linear_ist(struct
> > kvm *kvm,
> >  				      const struct
> > vgic_v5_ist_desc *ist,
> >  				      u32 __user *uaddr, size_t
> > nr_entries,
> >  				      u32 intid_type)
> >  {
> >  	__le32 h_iste;
> >  	size_t index;
> >  	int ret;
> >  
> >  	for (index = 0; index < nr_entries; index++) {
> >  		void *h_iste_addr = ist->base + index * ist-
> > >iste_size;
> >  
> >  		ret = get_user(h_iste, uaddr);
> >  		if (ret)
> >  			return ret;
> >  
> >  		ret = vgic_v5_restore_ist_entry(kvm, ist,
> > h_iste_addr,
> >  					h_iste, index,
> > intid_type);
> 
> [Severity: High]
> Should the SPI base offset be added to the index here?

No. Definitely not. That's a GICv3 thing, but not a GICv5 thing.

> 
> When restoring SPIs, passing the raw array index as the intid will
> track and
> inject interrupts starting at 0. Since hardware SPIs start at INTID
> 32, does
> this incorrectly inject SGIs and PPIs instead of the intended SPIs?

Except they don't with GICv5.

> 
> >  		if (ret)
> >  			return ret;
> >  
> >  		uaddr++;
> >  	}
> >  
> >  	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -1662,6 +1684,41 @@ static int
> > vgic_v5_restore_two_level_ist(struct kvm *kvm,
> >  					 const struct
> > vgic_v5_ist_desc *ist,
> >  					 u32 __user *uaddr, u32
> > intid_type)
> >  {
> 
> [ ... ]
> 
> >  		for (h_l2_index = 0; h_l2_index <
> > shape.l2_entries; h_l2_index++) {
> >  			void *h_iste_addr = h_l2_ist_base +
> >  					    h_l2_index * ist-
> > >iste_size;
> >  			u32 intid = h_l1_index * shape.l2_entries
> > + h_l2_index;
> >  
> >  			ret = get_user(h_iste, uaddr);
> >  			if (ret)
> >  				return ret;
> >  
> >  			ret = vgic_v5_restore_ist_entry(kvm, ist,
> > h_iste_addr,
> >  							h_iste,
> > intid,
> >  							intid_type
> > );
> 
> [Severity: High]
> Is there any validation missing for the generated LPI INTIDs?

No? Why would there be? Only LPIs in the table are valid. LPIs outside
of the table are not valid.

> 
> The iteration calculates intid starting from 0. If userspace sets
> pending
> bits for these lower indices, KVM would track them and inject them
> via the
> VDPEND instruction at EL2.

Yes? And?

> 
> Since valid LPI INTIDs must be >= 8192, could injecting lower IDs
> result in
> architecturally unpredictable hardware behavior?

No, that's GICv3 again!

> 
> >  			if (ret)
> >  				return ret;
> >  
> >  			uaddr++;
> >  		}
> >  	}
> >  
> >  	return 0;
> >  }
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 6af1264ed2371..153a8fcd4db7c 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> 
> [ ... ]
> 
> > @@ -1301,6 +1301,292 @@ void vgic_v5_save_state(struct kvm_vcpu
> > *vcpu)
> >  	__vgic_v5_save_ppi_state(cpu_if);
> >  	dsb(sy);
> >  }
> > +
> > +static int vgic_v5_irs_status_is_quiesced(u32 statusr)
> > +{
> > +	if (statusr & GICV5_IRS_SAVE_VM_STATUSR_Q)
> > +		return 0;
> > +
> > +	return -EBUSY;
> > +}
> > +
> > +static int vgic_v5_irs_is_quiesced(u16 vm_id)
> > +{
> > +	u32 statusr;
> > +	int ret;
> > +
> > +	ret = vgic_v5_irs_save_vm_op(vm_id, false, &statusr);
> 
> [Severity: High]
> Does this quiescence check potentially lose hardware state?
> 
> By calling vgic_v5_irs_save_vm_op() with save=false, it issues a
> SAVE_VMR
> command with Q=1 and S=0 (Quiesce but don't save).

That's not what this is doing. The Q bit is to query if the VM become
non-quiescent. It isn't stopping the save. We are tracking if it has
become non-quiescent since we started the save op (the last time we
wrote S=1).

> 
> If the VM had unexpectedly unquiesced (e.g., interrupted by
> hardware),
> forcing it to quiesce without saving would destroy its modified
> internal
> state before it can be exported to userspace.

Nope.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return vgic_v5_irs_status_is_quiesced(statusr);
> > +}
> > +
> > +static int vgic_v5_copy_ist_attr(struct kvm_device_attr *attr,
> > +				 struct kvm_vgic_v5_ist *ist_attr)
> > +{
> > +	void __user *uaddr = (void __user *)(unsigned long)attr-
> > >addr;
> > +
> > +	if (!uaddr)
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(ist_attr, uaddr, sizeof(*ist_attr)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vgic_v5_validate_ist_user_buffer(__u64 addr, __u64
> > size,
> > +					    size_t expected)
> > +{
> > +	if (!addr || size != expected)
> > +		return -EINVAL;
> 
> [Severity: Medium]
> Is there an issue here when restoring a VM configured with 0 SPIs?

That's not doable. We have a minimum of 32.

> 
> If userspace passes a size of 0 and an address of 0, the check if
> (!addr)
> will reject it with -EINVAL.
> 
> If userspace passes a dummy non-zero address to bypass this, it fails
> later
> in vgic_v5_get_spi_ist_desc():
> 
> arch/arm64/kvm/vgic/vgic-v5-tables.c:vgic_v5_get_spi_ist_desc() {
>     ...
>     if (!ist->base && userspace_buf)
>         return -ENOENT;
>     ...
> }
> 
> Does this catch-22 prevent live migration for VMs without SPIs?

No, because that's not a thing we support.

> 
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > +int vgic_v5_irs_save_ists(struct kvm *kvm, struct kvm_device_attr
> > *attr)
> > +{
> > +	struct kvm_vgic_v5_ist ist_attr;
> > +	u16 vm_id = vgic_v5_vm_id(kvm);
> > +	u32 statusr;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&kvm->lock);
> > +
> > +	if (kvm_trylock_all_vcpus(kvm)) {
> > +		mutex_unlock(&kvm->lock);
> > +		return -EBUSY;
> > +	}
> 
> [Severity: Critical]
> Is the return value of kvm_trylock_all_vcpus() handled correctly
> here?
> 
> The function returns true on successful lock acquisition and false on
> failure.

Where on earth did you pull that from? It returns 0 on success and -
EINTR on failure.

> Returning -EBUSY when it is true means we leak the vCPU locks
> and hang the VM.
> 
> Conversely, failing to acquire the locks lets us proceed lockless,
> and then
> unlocking unowned mutexes in the out_unlock path would cause issues.

This isn't an issue.

> 
> > +
> > +	mutex_lock(&kvm->arch.config_lock);
> 
> [ ... ]
> 
> > +static void vgic_v5_restore_cleanup(struct kvm *kvm,
> > +				    struct kvm_vcpu *vcpu,
> > +				    bool lpi_ist_allocated)
> > +{
> > +	/*
> > +	 * We are on the restore failure path, so we do a best-
> > effort
> > +	 * cleanup. These commands might fail, but at this stage
> > this is the
> > +	 * best we can realistically do.
> > +	 */
> > +	if (lpi_ist_allocated) {
> > +		if (!vgic_v5_send_command(vcpu,
> > VMTE_MAKE_INVALID))
> > +			vgic_v5_lpi_ist_free(kvm);
> > +	}
> 
> [Severity: High]
> Could this leave a dangling pointer behind?
> 
> When vgic_v5_lpi_ist_free() is called on a failure path, it frees the
> memory
> but does not set vmi->h_lpi_ist to NULL. If a subsequent teardown
> happens in
> vgic_v5_vmte_release(), it might observe the populated pointer and
> attempt
> to free it again, causing a double-free.

No. vgic_v5_lpi_ist_free() ultimately calls
vgic_v5_free_allocated_lpi_ist(), which clears vmi->h_lpi_ist after
freeing it (and clears the L2 pointer for two-level ISTs).
Consequently, a subsequent VMTE teardown will not attempt a second
free.

> 
> [Severity: Medium]
> Are we risking a memory leak by issuing a redundant invalidate
> command?
> 
> If we reach the failure path from a point where the VMTE is already
> invalid,
> this function will issue VMTE_MAKE_INVALID again because the
> vmte_invalid
> state is not passed in. If the hardware driver returns an error when
> asked
> to invalidate an already-invalid VMTE, vgic_v5_send_command() will
> fail.

The hardware has no way to indicate this error. It will just be a NOP.

> 
> This would skip the vgic_v5_lpi_ist_free() call and permanently leak
> the LPI
> IST memory.

Except it wouldn't. This is fine.

> 
> > +
> > +	vgic_v5_send_command(vcpu, VMTE_MAKE_VALID);
> > +}
> > +
> > +int vgic_v5_irs_restore_ists(struct kvm *kvm, struct
> > kvm_device_attr *attr)
> > +{
> > +	bool lpi_ist_allocated = false, vmte_invalid = false;
> > +	struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);
> 
> [Severity: Critical]
> What happens if userspace invokes this ioctl before creating any
> vCPUs?
> 
> kvm_get_vcpu(kvm, 0) would return NULL. Later, this NULL vcpu0 is
> passed to
> vgic_v5_send_command(), which dereferences it inside
> vgic_v5_vpe_db().
> 
> Should we verify vcpu0 exists before using it to prevent a potential
> NULL
> pointer dereference?

We reject an uninitialised vGIC before we reach this point. We require
vcpu0 as part of initialising the vGICv5 so we can't make it this far
without a vCPU0.

> 
> > +	struct kvm_vgic_v5_ist ist_attr;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&kvm->lock);
> > +
> > +	if (kvm_trylock_all_vcpus(kvm)) {
> > +		mutex_unlock(&kvm->lock);
> > +		return -EBUSY;
> > +	}
> 
> [Severity: Critical]
> As noted in vgic_v5_irs_save_ists(), does this correctly handle the
> boolean
> return value of kvm_trylock_all_vcpus()? It looks like this behaves
> in the
> exact opposite way as intended, leaking locks on success and running
> lockless on failure.

This again? No. Not an issue. See above.

> 
> > +
> > +	mutex_lock(&kvm->arch.config_lock);
> > +
> > +	if (!vgic_initialized(kvm)) {
> 
> [ ... ]
> 

Thanks,
Sascha