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

[sashiko-bot@kernel.org]

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 <Sascha.Bischoff@arm.com>
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?

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?

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

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.

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

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

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.

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

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?

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

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

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

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

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

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

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

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724104819.1296803-1-sascha.bischoff@arm.com?part=35