Re: [PATCH v5 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 3 potential issue(s) to consider:

New issues:
- [High] handle___vgic_v3_save_aprs erroneously overwrites the host's vgic_vmcr with a stale EL2 value.

Pre-existing issues:
- [Critical] Incomplete fix leaves identical hypervisor memory corruption vulnerability in GICv5 hypercalls.
- [Critical] The `handle___kvm_adjust_pc` hypercall blindly dereferences a host-provided `vcpu` pointer via `kern_hyp_va()` without validation.
--

--- Patch [6]: [PATCH v5 6/8] KVM: arm64: Minimise EL2's exposure of host VGIC state during world switch ---
commit bad346f74febb306b17aa647c2da5c6747345fe8
Author: Marc Zyngier <maz@kernel.org>

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

The host passes a vgic_v3_cpu_if pointer to the __vgic_v3_save_aprs and
__vgic_v3_restore_vmcr_aprs hypercalls, which EL2 dereferences wholesale.
This patch copies only vgic_vmcr and the active priority registers between
EL2's hyp-side state and the host vCPU to minimise exposure.

> 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_init_lrs(struct kvm_cpu_context *host_ctxt)
>  
>  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;

[Severity: High]
Does this overwrite the host's vgic_vmcr with a stale EL2 value?

When the guest performs an MMIO write to the GICv2 VMCR (emulated on a GICv3
host), the trap is handled at EL1, updating
host_vcpu->arch.vgic_cpu.vgic_v3.vgic_vmcr.

Later, if the vCPU is preempted or userspace schedules out,
__vgic_v3_save_aprs is invoked. Because __vgic_v3_save_aprs() does not read
VMCR from hardware, hyp_cpu_if->vgic_vmcr contains the stale value from the
previous guest exit.

This would silently clobber and destroy the EL1's MMIO emulation updates.

[ ... ]

This is a pre-existing issue, but I noticed a similar pattern in
handle___kvm_adjust_pc() in the same file:

arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___kvm_adjust_pc() {
        ...
        __kvm_adjust_pc(kern_hyp_va(vcpu));
}

[Severity: Critical]
Could this allow an EL1 host to corrupt hypervisor memory?

An EL1 host can call the __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc hypercall
and provide an arbitrary TTBR1 pointer. EL2's handle___kvm_adjust_pc()
applies kern_hyp_va() without bounds-checking or validation.

When __kvm_adjust_pc() is executed:

arch/arm64/kvm/hyp/exception.c:__kvm_adjust_pc() {
        if (vcpu_get_flag(vcpu, PENDING_EXCEPTION)) {
        ...

This dereferences the unvalidated pointer to read flags and modify memory,
giving EL1 the ability to increment 64-bit values in EL2 memory.


This is also a pre-existing issue, but a similar pattern remains in the
GICv5 counterparts:

arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v5_save_apr() {
        ...
        __vgic_v5_save_apr(kern_hyp_va(cpu_if));
}

arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v5_restore_vmcr_apr() {
        ...
        __vgic_v5_restore_vmcr_apr(kern_hyp_va(cpu_if));
}

[Severity: Critical]
Does this leave an identical memory corruption regression?

An EL1 host can call these hypercalls with an arbitrary TTBR1 pointer,
which is directly dereferenced without validation, leading to arbitrary
memory read/write against EL2.

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