Re: [PATCH v3 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:

Pre-existing issues:
- [Critical] The patch correctly fixes TOCTOU and arbitrary read/write vulnerabilities for GICv3 by preventing EL2 from directly dereferencing host-provided pointers in `handle___vgic_v3_save_aprs` and `handle___vgic_v3_restore_vmcr_aprs`. However, it leaves the exact same vulnerability unpatched in the GICv5 equivalents: `handle___vgic_v5_save_apr` and `handle___vgic_v5_restore_vmcr_apr`. These functions take a host-supplied pointer (`cpu_if`), which they pass directly to the inner save/restore functions via `kern_hyp_va`. Because `kern_hyp_va` is idempotent for hypervisor linear-map pointers, the untrusted host (which is an adversary in the pKVM threat model) can pass an arbitrary EL2 memory address. This allows the host to execute arbitrary memory reads and writes in the hypervisor (e.g., writing the value of `SYS_ICH_APR_EL2` to arbitrary EL2 memory or reading arbitrary memory into sysregs), completely bypassing pKVM isolation.
--

commit 815e460f9f860b3c06ec7eb259e1f2d75d196c33
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. That exposes the host's full VGIC emulation state to the
hypervisor, against pKVM's isolation goals.

> 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;
> +		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];
> +		}
> +	} else {
> +		__vgic_v3_save_aprs(&host_vcpu->arch.vgic_cpu.vgic_v3);
> +	}
>  }
>  
>  static void handle___vgic_v3_restore_vmcr_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;
> +
> +	host_vcpu = get_host_hyp_vcpus_from_vgic_v3_cpu_if(host_ctxt, 1,
> +							   &hyp_vcpu);
> +	if (!host_vcpu)
> +		return;
>  
> -	__vgic_v3_restore_vmcr_aprs(kern_hyp_va(cpu_if));
> +	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;
> +		host_cpu_if = &host_vcpu->arch.vgic_cpu.vgic_v3;
> +
> +		hyp_cpu_if->vgic_vmcr = host_cpu_if->vgic_vmcr;
> +		/* Should be a one-off */
> +		hyp_cpu_if->vgic_sre = (ICC_SRE_EL1_DIB |
> +					ICC_SRE_EL1_DFB |
> +					ICC_SRE_EL1_SRE);
> +		for (i = 0; i < ARRAY_SIZE(host_cpu_if->vgic_ap0r); i++) {
> +			hyp_cpu_if->vgic_ap0r[i] = host_cpu_if->vgic_ap0r[i];
> +			hyp_cpu_if->vgic_ap1r[i] = host_cpu_if->vgic_ap1r[i];
> +		}
> +
> +		__vgic_v3_restore_vmcr_aprs(hyp_cpu_if);
> +	} else {
> +		__vgic_v3_restore_vmcr_aprs(&host_vcpu->arch.vgic_cpu.vgic_v3);
> +	}
>  }

[Severity: Critical]
This is a pre-existing issue, but while this patch correctly fixes the TOCTOU
and arbitrary read/write vulnerabilities for GICv3, it seems the same pattern
remains unpatched in the GICv5 equivalents:

arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v5_save_apr() {
	DECLARE_REG(struct vgic_v5_cpu_if *, cpu_if, host_ctxt, 1);
	__vgic_v5_save_apr(kern_hyp_va(cpu_if));
}

arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v5_restore_vmcr_apr() {
	DECLARE_REG(struct vgic_v5_cpu_if *, cpu_if, host_ctxt, 1);
	__vgic_v5_restore_vmcr_apr(kern_hyp_va(cpu_if));
}

Since kern_hyp_va() is idempotent for hypervisor linear-map pointers, could
the untrusted host (which is an adversary in the pKVM threat model) pass an
arbitrary EL2 memory address here?

Would this allow the host to execute arbitrary memory reads and writes in the
hypervisor (for example, writing the value of SYS_ICH_APR_EL2 to arbitrary
EL2 memory or reading arbitrary memory into sysregs), which might bypass pKVM
isolation?

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