Re: [PATCH v2 7/8] KVM: arm64: Add primitives to flush/sync the VGIC state at EL2

[sashiko-bot@kernel.org]

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

New issues:
- [High] flush_hyp_vgic_state() fails to copy vgic_vmcr from the host vCPU to the hyp vCPU, causing host-side VMCR updates to be silently lost and subsequently clobbered.

Pre-existing issues:
- [High] flush_hyp_vcpu() unconditionally copies mdcr_el2 from the untrusted host vCPU, allowing the host to control debug trap configurations for protected VMs.
--

commit 5eaa1440f7e2f022736f913bec34aaa0cbd53a5d
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: Add primitives to flush/sync the VGIC state at EL2

pKVM performs its own world switch for protected VMs but has no primitives
to move the per-vCPU VGIC state between the host and hypervisor vCPU
contexts. Add flush_hyp_vgic_state() and sync_hyp_vgic_state() to handle
the required copying.

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index f3233ee343a39..2db56146ec493 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
[ ... ]
> @@ -104,6 +104,45 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
>  	*host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
>  }
>  
> +static void flush_hyp_vgic_state(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +	struct vgic_v3_cpu_if *host_cpu_if, *hyp_cpu_if;
> +	unsigned int used_lrs, i;
> +
> +	host_cpu_if	= &host_vcpu->arch.vgic_cpu.vgic_v3;
> +	hyp_cpu_if	= &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> +
> +	used_lrs	= host_cpu_if->used_lrs;
> +	used_lrs	= min(used_lrs, hyp_gicv3_nr_lr);
> +
> +	hyp_cpu_if->vgic_hcr	= host_cpu_if->vgic_hcr;
> +	/* Should be a one-off */

[Severity: High]
Does flush_hyp_vgic_state() need to copy vgic_vmcr from the host vCPU to the
hyp vCPU here?

For GICv2-on-GICv3 emulation, guest writes to GICC_VMCR and GICC_CTLR trap
to the host (EL1) as MMIO. 

The host updates the host_vcpu's vgic_vmcr and resumes the guest loop
without a full vcpu_put/load cycle. 

By omitting the copy here, the hypervisor will re-enter the guest with the
old hardware VMCR state.

> +	hyp_cpu_if->vgic_sre	= (ICC_SRE_EL1_DIB |
> +				   ICC_SRE_EL1_DFB |
> +				   ICC_SRE_EL1_SRE);
> +	hyp_cpu_if->used_lrs	= used_lrs;
> +
> +	for (i = 0; i < used_lrs; i++)
> +		hyp_cpu_if->vgic_lr[i] = host_cpu_if->vgic_lr[i];
> +}
> +
> +static void sync_hyp_vgic_state(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +	struct vgic_v3_cpu_if *host_cpu_if, *hyp_cpu_if;
> +	unsigned int i;
> +
> +	host_cpu_if	= &host_vcpu->arch.vgic_cpu.vgic_v3;
> +	hyp_cpu_if	= &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> +
> +	host_cpu_if->vgic_hcr = hyp_cpu_if->vgic_hcr;
> +	host_cpu_if->vgic_vmcr = hyp_cpu_if->vgic_vmcr;

[Severity: High]
Will this unconditionally overwrite the host's vgic_vmcr with the hypervisor's
stale copy?

Since flush_hyp_vgic_state() doesn't bring the updated vgic_vmcr into the
hyp vCPU on entry, syncing it back to the host here appears to permanently
clobber the guest's MMIO write.

This missing synchronization could break GICv2 priority masking and group
enables.

> +
> +	for (i = 0; i < hyp_cpu_if->used_lrs; i++)
> +		host_cpu_if->vgic_lr[i] = hyp_cpu_if->vgic_lr[i];
> +}
[ ... ]
> @@ -152,13 +191,7 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)

[Severity: High]
This is a pre-existing issue, but does flush_hyp_vcpu() unconditionally copy
mdcr_el2 from the untrusted host vCPU?

Currently, flush_hyp_vcpu() contains:

    hyp_vcpu->vcpu.arch.mdcr_el2 = host_vcpu->arch.mdcr_el2;

In pKVM, the host memory is untrusted. By manipulating the host_vcpu's
mdcr_el2, could a compromised host configure debug traps (e.g., MDCR_EL2.TDA,
TDE) for protected VMs? 

This would bypass the pKVM threat model, which requires trap configuration
registers to be initialized in an EL2-private context to ensure guest
confidentiality and integrity.

>  
>  	hyp_vcpu->vcpu.arch.vsesr_el2	= host_vcpu->arch.vsesr_el2;
>  
> -	hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3 = host_vcpu->arch.vgic_cpu.vgic_v3;
> -
> -	/* Bound used_lrs by the number of implemented list registers. */
> -	hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3.used_lrs =
> -		min_t(unsigned int,
> -		      hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3.used_lrs,
> -		      hyp_gicv3_nr_lr);
> +	flush_hyp_vgic_state(hyp_vcpu);
>  
>  	hyp_vcpu->vcpu.arch.pid = host_vcpu->arch.pid;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619070719.812227-1-tabba@google.com?part=7