Re: [PATCH v3 36/40] KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Blindly freeing LPI IST pages and directly modifying active VMTE memory on a failed restore cleanup.
- [High] No mechanism to un-quiesce the VM after a cancelled migration, permanently bricking the guest.
- [High] Restoring LPI IST processes INTIDs < 8192, injecting invalid interrupts into the hardware.
- [High] Unprivileged userspace can trigger WARN_ON_ONCE kernel warnings by calling IST save/restore ioctls at unexpected lifecycle states.
- [Medium] Truncation of 64-bit VMTE bitwise AND result into 32-bit `int` incorrectly flags valid ISTs as invalid.
--

commit b7ddc5cb1b3a60a4389e44b1be34dab0a54a57d5
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

This patch implements save and restore mechanisms for the GICv5 Interrupt
State Tables (ISTs). It allocates and synchronizes hypervisor memory for the
SPI IST and guest memory for the LPI IST during migration. It also handles
quiescing the VM and managing pending interrupts during the transition.

> diff --git a/arch/arm64/kvm/vgic/vgic-irs-v5.c b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> --- a/arch/arm64/kvm/vgic/vgic-irs-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c
[ ... ]
> @@ -1027,6 +1027,26 @@ int kvm_vgic_v5_irs_init(struct kvm *kvm, unsigned int nr_spis)
>  	return 0;
>  }
>  
> +int vgic_v5_irs_lpi_ist_id_bits(struct kvm *kvm, unsigned int *id_bits)
> +{
> +	struct vgic_v5_irs *irs = kvm->arch.vgic.vgic_v5_irs_data;
> +
> +	if (WARN_ON_ONCE(!irs))
> +		return -ENXIO;

[Severity: High]
Could unprivileged userspace intentionally trigger this warning?

If a process calls the IST restore ioctl without properly configuring an IRS
address first, the pointer will be missing. This might allow a malicious
guest to trigger host kernel panics if panic_on_warn is enabled.

[ ... ]
> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
[ ... ]
> +static int vgic_v5_read_vm_ist_desc(struct kvm *kvm, unsigned int section,
> +				    struct vgic_v5_ist_desc *ist)
> +{
> +	u32 vm_id = vgic_v5_vm_id(kvm);
> +	struct vmtl2_entry *vmte;
> +	u64 vmte_ist_section;
> +
> +	vmte = vgic_v5_get_l2_vmte(vm_id);
> +	if (IS_ERR(vmte))
> +		return PTR_ERR(vmte);
> +
> +	vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +	vmte_ist_section = le64_to_cpu(READ_ONCE(vmte->val[section]));
> +
> +	ist->id_bits = FIELD_GET(GICV5_VMTEL2E_IST_ID_BITS, vmte_ist_section);
> +	ist->istsz = FIELD_GET(GICV5_VMTEL2E_IST_ISTSZ, vmte_ist_section);
> +	ist->l2sz = FIELD_GET(GICV5_VMTEL2E_IST_L2SZ, vmte_ist_section);
> +	ist->iste_size = GICV5_ISTE_SIZE(ist->istsz);
> +
> +	return vmte_ist_section & GICV5_VMTEL2E_IST_VALID;
> +}

[Severity: Medium]
Does the return type here truncate the result?

The function returns an int, but evaluates a bitwise AND on a 64-bit value.
If GICV5_VMTEL2E_IST_VALID is at bit 32 or higher, the cast to a 32-bit
signed integer would truncate it to 0, incorrectly marking valid ISTs as
invalid.

> +static int vgic_v5_get_spi_ist_desc(struct kvm *kvm, bool userspace_buf,
> +				    struct vgic_v5_ist_desc *ist)
> +{
> +	u32 vm_id = vgic_v5_vm_id(kvm);
> +	int ret;
> +
> +	memset(ist, 0, sizeof(*ist));
> +
> +	ist->vmi = xa_load(&vm_info, vm_id);
> +	if (WARN_ON_ONCE(!ist->vmi))
> +		return -ENXIO;

[Severity: High]
Is there a similar risk of userspace triggering this warning as seen in
vgic_v5_irs_lpi_ist_id_bits()?

If the save/restore ioctls are called before the VM runs and vm_info is
populated, ist->vmi will be unpopulated.

[ ... ]
> +static int vgic_v5_restore_linear_lpi_ist(struct kvm *kvm,
> +					  const struct vgic_v5_ist_desc *ist,
> +					  gpa_t g_entry_addr)
> +{
> +	size_t h_l2_index, h_l2_entries;
> +	__le32 h_iste;
> +	int ret;
> +
> +	h_l2_entries = BIT(ist->id_bits);
> +
> +	for (h_l2_index = 0; h_l2_index < h_l2_entries; h_l2_index++) {
> +		void *h_iste_addr = ist->base + h_l2_index * ist->iste_size;
> +
> +		ret = kvm_read_guest(kvm, g_entry_addr, &h_iste,
> +				     sizeof(h_iste));
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Sanitise the IST, clearing HWU & pending fields. Pending
> +		 * state is later replayed via GIC VDPEND.
> +		 */
> +		ret = vgic_v5_process_iste(&h_iste, &ist->vmi->pending_irqs,
> +					   h_l2_index, GICV5_HWIRQ_TYPE_LPI);

[Severity: High]
Could this inject invalid interrupts into the hardware?

The loop iterates from index 0 and processes all pending bits in the guest
provided LPI IST. Since LPI IDs must be 8192 or greater, queuing indices
0-8191 as pending LPIs and injecting them via VDPEND violates the
architecture and could cause unpredictable hardware behavior.

[ ... ]
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
[ ... ]
> @@ -405,6 +417,27 @@ static int vgic_v5_irs_set_up_vpe(u16 vm_id, u16 vpe_id,
>  	return 0;
>  }
>  
> +static int vgic_v5_irs_save_vm_op(u16 vm_id, bool save, u32 *statusr)
> +{
> +	u64 save_vmr;
> +	int ret;
> +
> +	save_vmr = FIELD_PREP(GICV5_IRS_SAVE_VMR_VM_ID, vm_id);
> +	save_vmr |= GICV5_IRS_SAVE_VMR_Q;
> +	save_vmr |= FIELD_PREP(GICV5_IRS_SAVE_VMR_S, save);

[Severity: High]
Is there a path to un-quiesce the VM after this operation completes?

The save operation unconditionally sets GICV5_IRS_SAVE_VMR_Q, but there
does not seem to be a corresponding mechanism anywhere to write the bit as
0. If a save is performed to checkpoint state and the VM continues execution,
will it be permanently prevented from receiving interrupts?

[ ... ]
> +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) {
> +		vgic_v5_send_command(vcpu, VMTE_MAKE_INVALID);
> +		vgic_v5_lpi_ist_free(kvm);
> +	}

[Severity: High]
Are we risking memory corruption if VMTE_MAKE_INVALID fails here?

If the invalidation command fails, the hardware might still consider the VMTE
valid. Calling vgic_v5_lpi_ist_free() immediately afterward frees the IST
pages and zeroes the active VMTE in memory while the hardware is still
using it.

[ ... ]

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