Re: [PATCH v6 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] Removal of hcr_el2 synchronization breaks virtual SError injection, causing an infinite loop of virtual SErrors.
- [High] Asynchronous host-side modifications to vCPU system registers (e.g., PMU overflow updating PMOVSSET_EL0) are permanently lost due to lazy state sync.

Pre-existing issues:
- [Critical] The hypervisor unconditionally overwrites a protected guest's register context with the untrusted host context on every entry, and leaks it back to the host on exit.
--

--- Patch [8]: [PATCH v6 8/8] KVM: arm64: Implement lazy vCPU state sync for non-protected guests ---
commit 89d46644a5cb6e03eee6a1ef5d530db27a9d6cdb
Author: Fuad Tabba <fuad.tabba@linux.dev>
Subject: KVM: arm64: Implement lazy vCPU state sync for non-protected guests

This commit defers the register context copy for non-protected guests between
the host and hypervisor. The state is only flushed to the hypervisor if it is
marked dirty by the host on entry, and copied back to the host on exit only
when needed. Protected guests continue to have their state copied
unconditionally.

> 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 (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 IRQ exits result in dropping asynchronous
host modifications to vCPU system registers?

When an external IRQ triggers during guest execution, handle_exit_pkvm_state()
clears the dirty flag. If the host's IRQ handler asynchronously updates the
host's vcpu->arch.ctxt (for example, kvm_pmu_perf_overflow() setting
PMOVSSET_EL0), flush_hyp_vcpu() will skip syncing this updated state on the
next guest entry.

When the guest later traps, handle_exit_pkvm_state() calls
__pkvm_vcpu_sync_state(), which overwrites the host's updated context with the
hypervisor's stale context, permanently erasing the 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 this unconditionally overwrite the
protected guest's register context with the untrusted host context?

In flush_hyp_vcpu(), this line allows the host EL1 to arbitrarily modify the
protected guest's general purpose and system registers, bypassing pKVM's guest
confidentiality and integrity guarantees.

> +	}
>  
>  	/* __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]
Does removing the synchronization of hcr_el2 break virtual SError injection and
cause an infinite loop of virtual SErrors?

When the host injects a virtual SError by setting HCR_VSE in its local
hcr_el2 copy, the guest takes the SError, and the hardware clears HCR_VSE in
the hypervisor's active hcr_el2.

Without this line, the hypervisor's cleared state is never synchronized back
to the host. The host's copy permanently retains HCR_VSE=1, and on every
subsequent guest entry, flush_hyp_vcpu() will copy HCR_VSE=1 back to the
hypervisor, endlessly re-injecting the SError.

> +	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 does this leak the protected guest's
registers back to the untrusted host?

Similar to the issue in flush_hyp_vcpu(), this line in sync_hyp_vcpu() copies
the protected guest's register context to the untrusted host on exit, which
violates pKVM's threat model guarantees.

> +	} 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/20260715081238.1891918-1-fuad.tabba@linux.dev?part=8