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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch fixes an arbitrary host pointer dereference in GICv3 hypercalls but completely misses the identical vulnerabilities in GICv5 hypercalls (`handle___vgic_v5_save_apr` and `handle___vgic_v5_restore_vmcr_apr`).
- [Critical] Unvalidated host pointer dereference in `handle___kvm_adjust_pc` allows arbitrary memory modification in EL2.
--

--- Patch [6]: [PATCH v7 6/8] KVM: arm64: Minimise EL2's exposure of host VGIC state during world switch ---
commit def324be61872b6056efce4e3eb9ff57eb9a9e10
Author: Marc Zyngier <[email protected]>
Subject: KVM: arm64: Minimise EL2's exposure of host VGIC state during world switch

This commit restricts EL2's exposure to host VGIC state by recovering the
host vCPU securely rather than directly dereferencing the host-provided
vgic_v3_cpu_if pointer in __vgic_v3_save_aprs and
__vgic_v3_restore_vmcr_aprs. It introduces a helper to fetch the host
vCPU using container_of() and copies only the necessary state.

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

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does the same regression exist for the
GICv5 hypercalls?

The patch replaces the direct kern_hyp_va() dereference with a secure lookup
for GICv3. However, it appears the same pattern remains in
handle___vgic_v5_save_apr() and handle___vgic_v5_restore_vmcr_apr():

handle___vgic_v5_save_apr()
    __vgic_v5_save_apr(kern_hyp_va(cpu_if));

Can a compromised host pass an arbitrary physical address here, causing
__vgic_v5_save_apr() to write the hardware value of SYS_ICH_APR_EL2 to that
chosen address, breaking EL2 isolation?

[Severity: Critical]
This is also a pre-existing issue, but does handle___kvm_adjust_pc() have a
similar unvalidated host pointer dereference?

In arch/arm64/kvm/hyp/nvhe/hyp-main.c:

static void handle___kvm_adjust_pc(struct kvm_cpu_context *host_ctxt)
{
	DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1);

	__kvm_adjust_pc(kern_hyp_va(vcpu));
}

If the host kernel passes a crafted vcpu pointer, could it point to an
arbitrary memory region where the iflags offset has the INCREMENT_PC bit set?

This would cause the hypervisor to execute:

arch/arm64/kvm/hyp/exception.c:__kvm_adjust_pc() {
    ...
    } else if (vcpu_get_flag(vcpu, INCREMENT_PC)) {
        kvm_skip_instr(vcpu);
        ...
}

Could this allow an attacker to increment arbitrary EL2 memory by adding 4 to
the value at the regs.pc offset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6