Re: [PATCH v1 11/11] KVM: arm64: Implement lazy vCPU state sync for non-protected guests

[sashiko-bot@kernel.org]

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

New issues:
- [High] Lazy vCPU state sync bypasses vcpu_mode_is_bad_32bit() check on non-trap VM exits due to stale CPSR.
- [High] Exceptions injected by the host and other context modifications during the KVM_RUN loop are silently dropped, causing infinite VM exit loops or ignored requests.

Pre-existing issues:
- [Critical] Protected VM register state is unconditionally copied between the hypervisor and untrusted host, violating pKVM confidentiality and integrity guarantees.
--

commit 96ecd36038c29097f333a40bf0eebd7b62bffc92
Author: tabba@google.com <tabba@google.com>
Subject: KVM: arm64: Implement lazy vCPU state sync for non-protected guests

This commit implements lazy synchronization of vCPU register state for
non-protected guests running under pKVM. It defers copying the context from
the host to the hypervisor until the host marks it dirty, and from the
hypervisor to the host until the host requires it. The PC is synced on every
exit to maintain accurate tracepoints.

> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 54aedf93c78b6..dccc3786548b0 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
[ ... ]
> @@ -489,6 +504,13 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  /* For exit types that need handling before we can be preempted */
>  void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
>  {
> +	/*
> +	 * We just exited, so the state is clean from a hypervisor
> +	 * perspective.
> +	 */
> +	if (is_protected_kvm_enabled())
> +		vcpu_clear_flag(vcpu, PKVM_HOST_STATE_DIRTY);
> +

[Severity: High]
When a guest exits (such as for ARM_EXCEPTION_IL or an IRQ), this clears the
PKVM_HOST_STATE_DIRTY flag.

If the host handles the exit or processes pending requests (like
KVM_REQ_VCPU_RESET or injecting undefined/SErrors), those operations modify
the host's copy of the guest register context (vcpu->arch.ctxt).

However, since these paths bypass handle_trap_exceptions(), the
PKVM_HOST_STATE_DIRTY flag is never set again.

On the next guest entry, flush_hyp_vcpu() at EL2 will see the dirty flag is
clear and skip calling __flush_hyp_vcpu().

Will this cause exceptions injected by the host and other context modifications
made during the KVM_RUN loop to be silently dropped? For ARM_EXCEPTION_IL,
does this mean the hypervisor will re-enter the guest using the old,
unmodified register state, causing an instant re-trap?

>  	if (ARM_SERROR_PENDING(exception_index)) {
>  		if (this_cpu_has_cap(ARM64_HAS_RAS_EXTN)) {
>  			u64 disr = kvm_vcpu_get_disr(vcpu);
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 23e644c24a03a..02383b372258f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
[ ... ]
> @@ -168,7 +211,17 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  	fpsimd_sve_flush();
>  	flush_debug_state(hyp_vcpu);
>  
> -	hyp_vcpu->vcpu.arch.ctxt	= host_vcpu->arch.ctxt;
> +	/*
> +	 * If we deal with a non-protected guest and the state is potentially
> +	 * dirty (from a host perspective), copy the state back into the hyp
> +	 * vcpu.
> +	 */
> +	if (!pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
> +		if (vcpu_get_flag(host_vcpu, PKVM_HOST_STATE_DIRTY))
> +			__flush_hyp_vcpu(hyp_vcpu);
> +	} else {
> +		hyp_vcpu->vcpu.arch.ctxt = host_vcpu->arch.ctxt;

[Severity: Critical]
This is a pre-existing issue, but I have a question about this copy operation.

This executes at EL2 and copies the entire vcpu.arch.ctxt from host_vcpu
(memory accessible to the untrusted host EL1) into the protected VM's
private state.

Could this violate guest integrity by allowing the host to hijack the VM's
control flow?

> +	}
>  
>  	hyp_vcpu->vcpu.arch.mdcr_el2	= host_vcpu->arch.mdcr_el2;
>  	hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWI | HCR_TWE);
[ ... ]
> @@ -191,9 +244,11 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  	fpsimd_sve_sync(&hyp_vcpu->vcpu);
>  	sync_debug_state(hyp_vcpu);
>  
> -	host_vcpu->arch.ctxt		= hyp_vcpu->vcpu.arch.ctxt;
> -
> -	host_vcpu->arch.hcr_el2		= hyp_vcpu->vcpu.arch.hcr_el2;
> +	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu))
> +		host_vcpu->arch.ctxt = hyp_vcpu->vcpu.arch.ctxt;

[Severity: Critical]
This is a pre-existing issue, but I have a similar question about copying
the protected guest's register state back to the host here.

Does copying hyp_vcpu->vcpu.arch.ctxt back into the untrusted
host_vcpu->arch.ctxt violate pKVM confidentiality guarantees by leaking
the state to the host?

> +	else
> +		/* Keep the PC current for the kvm_exit tracepoint (lazy ctxt sync). */
> +		host_vcpu->arch.ctxt.regs.pc = hyp_vcpu->vcpu.arch.ctxt.regs.pc;

[Severity: High]
Since handle_trap_exceptions() is only called for traps, an exit like
ARM_EXCEPTION_IRQ won't trigger the __pkvm_vcpu_sync_state hypercall.

Because this code only synchronizes the pc field for non-protected guests,
vcpu->arch.ctxt.regs.pstate (CPSR) remains stale in the host vCPU context.

Does this lazy synchronization leave the host with a stale CPSR on non-trap
exits?

If the guest transitioned to an illegal 32-bit mode immediately before the
IRQ was taken, would the KVM_RUN inner loop evaluate vcpu_mode_is_bad_32bit()
using this stale CPSR, bypassing KVM's architectural protection against illegal
AArch32 execution?

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

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