Re: [PATCH v4 6/8] KVM: arm64: Minimise EL2's exposure of host VGIC state during world switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Userspace KVM_SET_ONE_REG modifications to ICC_VMCR_EL1 and Active Priority Registers (APRs) are silently overwritten and lost on vcpu_put() under pKVM.
--

commit 8bda67fe364c44ad574acb3da5de4bf16e0ccbbf
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: Minimise EL2's exposure of host VGIC state during world switch

This commit avoids exposing the host's entire vgic_v3_cpu_if to EL2 by
copying only the necessary vgic_vmcr and active priority registers during
save and restore operations.

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 45d717889f6ea..f3233ee343a39 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c

[ ... ]

> @@ -508,16 +520,63 @@ static void handle___vgic_v3_save_aprs(struct kvm_cpu_context *host_ctxt)
>  {
> -	DECLARE_REG(struct vgic_v3_cpu_if *, cpu_if, host_ctxt, 1);
> +	struct pkvm_hyp_vcpu *hyp_vcpu;
> +	struct kvm_vcpu *host_vcpu;
>  
> -	__vgic_v3_save_aprs(kern_hyp_va(cpu_if));
> +	host_vcpu = get_host_hyp_vcpus_from_vgic_v3_cpu_if(host_ctxt, 1,
> +							   &hyp_vcpu);
> +	if (!host_vcpu)
> +		return;
> +
> +	if (unlikely(hyp_vcpu)) {
> +		struct vgic_v3_cpu_if *hyp_cpu_if, *host_cpu_if;
> +		int i;
> +
> +		hyp_cpu_if = &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> +		__vgic_v3_save_aprs(hyp_cpu_if);
> +
> +		host_cpu_if = &host_vcpu->arch.vgic_cpu.vgic_v3;
> +		host_cpu_if->vgic_vmcr = hyp_cpu_if->vgic_vmcr;
> +		for (i = 0; i < ARRAY_SIZE(host_cpu_if->vgic_ap0r); i++) {
> +			host_cpu_if->vgic_ap0r[i] = hyp_cpu_if->vgic_ap0r[i];
> +			host_cpu_if->vgic_ap1r[i] = hyp_cpu_if->vgic_ap1r[i];
> +		}

[Severity: High]
Does this sequence risk clobbering userspace writes to ICC_VMCR_EL1 and the
Active Priority Registers?

When userspace modifies the GIC CPU interface via KVM_SET_ONE_REG, the
ioctl is wrapped by vcpu_load() and vcpu_put().

Under pKVM, vcpu_load() calls __vgic_v3_restore_vmcr_aprs, which restores
the hardware with the current values.

The ioctl then updates host_cpu_if in memory with the new userspace values.

Finally, vcpu_put() issues the __vgic_v3_save_aprs hypercall.

Since the hardware and hyp_cpu_if only have the pre-ioctl stale values,
will this explicit copy back to host_cpu_if overwrite the new values set
by userspace? This could cause state restoration or live migration to
silently lose these register updates.

> +	} else {
> +		__vgic_v3_save_aprs(&host_vcpu->arch.vgic_cpu.vgic_v3);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706095927.560795-1-fuad.tabba@linux.dev?part=6