Re: [PATCH v4 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] The commit silently removes the synchronization of hcr_el2 from the hyp vCPU back to the host vCPU on guest exit, breaking host-injected runtime signals like virtual SErrors.
- [High] Host-side modifications to a non-protected guest's system registers via asynchronous KVM requests (e.g. KVM_REQ_PMU) are silently discarded because the dirty flag is cleared on IRQ exits.

Pre-existing issues:
- [Critical] pKVM completely fails to isolate the register state of protected guests by unconditionally copying the full execution context between the untrusted host and the hypervisor on every world switch.
--

commit 417bef11bf8b55291221bae4776c5695349c903d
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, even when the host never
inspects it. Defer the copy: on entry, flush the host context into the
hyp vCPU only when the host marked it dirty (PKVM_HOST_STATE_DIRTY); on
exit, leave it in the hyp vCPU and copy it back only when the host needs
it, via a __pkvm_vcpu_sync_state hypercall or at vcpu put.

> 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]
Does clearing the dirty flag on other exits (like IRQ) blind the lazy sync to
subsequent host-side state modifications?

If a non-protected guest exits due to a physical IRQ, the PKVM_HOST_STATE_DIRTY
flag is cleared here. If the host IRQ handler then updates guest state (e.g. via
PMU overflow) and queues an asynchronous KVM request, the next time the guest is
entered, flush_hyp_vcpu() will skip flush_hyp_vcpu_state() because the dirty
flag is clear.

Will this result in the guest running with stale state, discarding the host's
modifications?

> +	}
> +}
> +
>  /* For exit types that need handling before we can be preempted */
>  void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
>  {
> +	handle_exit_pkvm_state(vcpu, exception_index);
> +
>  	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 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 pKVM seems to completely fail to isolate the
register state of protected guests by unconditionally copying the untrusted host
context into the protected hyp vCPU context here on every world switch.

Can this allow the host kernel, which is treated as an adversary against guest
confidentiality and integrity, to arbitrarily inject state into the protected VM?

> +	}
>  
>  	/* __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;
> +	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 similar to the copy on entry, unconditionally
copying the protected hyp vCPU context back to the untrusted host context here
leaks the guest's execution context.

Does this break the core pKVM confidentiality guarantees by allowing the host
to read all guest registers?

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

[Severity: High]
Does removing the synchronization of hcr_el2 from the hyp vCPU back to the host
vCPU break host-injected runtime signals like virtual SErrors?

When a virtual SError is injected via HCR_VSE, the guest hardware clears the
bit upon delivery. Previously, this cleared state was synced back to the host
here in sync_hyp_vcpu().

Since the sync is now removed, won't the host's hcr_el2 retain the stale HCR_VSE
bit forever? On every subsequent guest entry, flush_hyp_vcpu() will OR this
stale bit back into the hyp vCPU's hcr_el2, which seems like it would cause an
infinite loop of spurious virtual SError injections into the guest.

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

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