Re: [PATCH v3 8/8] 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] Asynchronous host modifications to non-protected guest sysregs (e.g., PMU overflows updating PMOVSSET_EL0) are lost because lazy vCPU state sync clears the dirty flag on IRQ exits and later overwrites the host's updates with stale hypervisor state.
- [High] The patch removes the synchronization of `hcr_el2` from the hypervisor back to the host on guest exit, breaking the delivery tracking of virtual SErrors and causing infinite virtual SError loops.

Pre-existing issues:
- [Critical] pKVM completely breaks register confidentiality and integrity for protected VMs by copying the entire execution context (`arch.ctxt`) to and from untrusted host memory on every world switch.
--

commit 44f38b89413b112e6bbd7b360cf03c7937ed43a4
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: Implement lazy vCPU state sync for non-protected guests

pKVM copies a non-protected guest's register context between the host
and the hypervisor on every world switch. This commit defers the copy
by flushing context on entry only if marked dirty, and syncing back
on exit only when requested.

> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 54aedf93c78b6..29108e5c0206e 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
[ ... ]
> @@ -486,9 +486,32 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  	}
>  }
>  
> +static void handle_exit_pkvm_state(struct kvm_vcpu *vcpu, int exception_index)
> +{
> +	int exception_code = ARM_EXCEPTION_CODE(exception_index);
> +
> +	if (!is_protected_kvm_enabled() || kvm_vm_is_protected(vcpu->kvm))
> +		return;
> +
> +	/*
> +	 * Sync the context back when the host will read (trap) or write
> +	 * (SError) it. Preempt-off here, so the loaded hyp vCPU is stable.
> +	 */
> +	if (exception_code == ARM_EXCEPTION_TRAP ||
> +	    exception_code == ARM_EXCEPTION_EL1_SERROR ||
> +	    ARM_SERROR_PENDING(exception_index)) {
> +		kvm_call_hyp_nvhe(__pkvm_vcpu_sync_state);
> +		vcpu_set_flag(vcpu, PKVM_HOST_STATE_DIRTY);
> +	} else {
> +		vcpu_clear_flag(vcpu, PKVM_HOST_STATE_DIRTY);

[Severity: High]
Is it possible for asynchronous host updates to be lost here? If a physical
IRQ (like a PMU overflow) causes a guest exit, this else branch clears
PKVM_HOST_STATE_DIRTY.

If the host IRQ handler then updates PMOVSSET_EL0 in the host's
vcpu->arch.ctxt, the next guest entry in flush_hyp_vcpu() will skip
copying sys_regs to the hyp vCPU. The guest will see stale state.

If the guest later triggers a TRAP exit, __pkvm_vcpu_sync_state() is
called, which copies the stale hypervisor sys_regs back to the host,
permanently erasing the host's asynchronous update.

> +	}
> +}
[ ... ]
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 2db56146ec493..4a8445b636e60 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
[ ... ]
> @@ -172,7 +214,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_state(hyp_vcpu);
> +	} else {
> +		hyp_vcpu->vcpu.arch.ctxt = host_vcpu->arch.ctxt;

[Severity: Critical]
This is a pre-existing issue, but does trusting arch.ctxt violate the pKVM
threat model? Copying the entire execution context directly from untrusted
host memory into the protected guest context on entry breaks integrity,
allowing the host to arbitrarily modify a protected guest's registers.

> +	}
>  
>  	/* __hyp_running_vcpu must be NULL in a guest context. */
>  	hyp_vcpu->vcpu.arch.ctxt.__hyp_running_vcpu = NULL;
[ ... ]
> @@ -203,9 +255,13 @@ 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;

[Severity: High]
Could removing the hcr_el2 sync result in infinite virtual SError loops?

When the host injects a virtual SError by setting HCR_VSE, flush_hyp_vcpu()
flows this bit to the hyp vCPU. The hardware clears the physical
HCR_EL2.VSE bit when the guest takes the virtual SError, and KVM clears it
in the hyp vCPU's hcr_el2.

Without syncing this back to the host here, the host's hcr_el2 retains
the stale HCR_VSE=1. On the next entry, flush_hyp_vcpu() will re-apply the
stale HCR_VSE bit and re-inject the SError continuously.

> +	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 can writing arch.ctxt out to the host
leak sensitive register state? For protected VMs, writing the private guest
context directly out to the untrusted host on exit breaks confidentiality.

> +	} else {
> +		/* Keep PC (tracepoint) and PSTATE (vcpu_mode_is_bad_32bit) current. */
> +		host_vcpu->arch.ctxt.regs.pc = hyp_vcpu->vcpu.arch.ctxt.regs.pc;
> +		host_vcpu->arch.ctxt.regs.pstate = hyp_vcpu->vcpu.arch.ctxt.regs.pstate;
> +	}

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