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

New issues:
- [High] handle___vgic_v3_save_aprs silently overwrites host modifications to vgic_vmcr with a stale value.

Pre-existing issues:
- [Critical] The patch addresses the host-pointer trust vulnerability for GICv3 but leaves the identical vulnerability open for GICv5.
--

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

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

This commit modifies the world switch to stop EL2 from dereferencing the host's
vgic_v3_cpu_if pointer wholesale. It copies only vgic_vmcr and active priority 
registers to the hyp-side 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_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 assignment overwrite host modifications to vgic_vmcr with a stale
value?

When userspace modifies a VMCR field (like SYS_ICC_PMR_EL1) via
KVM_SET_ONE_REG, the execution flow modifies the host state:

kvm_vcpu_ioctl()
  vcpu_load()
    handle___vgic_v3_restore_vmcr_aprs()
  // ioctl modifies host_cpu_if->vgic_vmcr
  vcpu_put()
    handle___vgic_v3_save_aprs()

Since __vgic_v3_save_aprs() does not read ICH_VMCR_EL2 from the hardware,
hyp_cpu_if->vgic_vmcr contains the old value. Does copying it back to
host_cpu_if->vgic_vmcr here cause the userspace changes to be lost?

> +		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);
> +	}
>  }
[ ... ]

[Severity: Critical]
This is a pre-existing issue and was not introduced by the patch under review,
but does the same pattern remain open for GICv5?

Looking at handle___vgic_v5_save_apr():

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

And similarly in handle___vgic_v5_restore_vmcr_apr():

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 these handlers apply kern_hyp_va() to an untrusted host pointer without
validation, and kern_hyp_va() is idempotent for already-hyp-tagged pointers,
could a malicious host provide a pointer to EL2 private metadata?

Could this allow the host to arbitrarily read or write EL2 memory?

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