Re: [PATCH v5 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 back to the host prevents clearance of `HCR_VSE`, causing infinite virtual SError storms in the guest.
- [High] Lazy state sync skips synchronization and clears `PKVM_HOST_STATE_DIRTY` on IRQ and IL exits, silently discarding host-side context modifications.

Pre-existing issues:
- [Critical] Protected guest register state is unconditionally leaked to and overwritten by untrusted host memory (`host_vcpu->arch.ctxt`) on every world switch.
--

--- Patch [8]: [PATCH v5 8/8] KVM: arm64: Implement lazy vCPU state sync for non-protected guests ---
commit e68e83fa70438b2538d1ea836d971c6d52ffb760
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; on exit, leave it in the
hyp vCPU and copy it back only when the host needs it.

> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> --- 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)
> +{

[ ... ]

> +	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]
If a non-protected guest exits due to an IRQ (such as a PMU overflow), this
clears PKVM_HOST_STATE_DIRTY. If the host kernel then handles the IRQ and
modifies the guest context (for example, setting PMOVSSET_EL0 for the
overflow or applying a reset like KVM_REQ_SUSPEND), these updates appear to
be lost.

On the next entry, flush_hyp_vcpu() will skip copying the host state because
the dirty flag is clear. Then, on a subsequent exit or vcpu_put,
handle___pkvm_vcpu_put() will reverse-sync the stale hyp state back over the
host state. Does this clear flag path silently discard host-side context
modifications?

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> --- 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 doesn't this unconditionally overwrite the
protected guest's private EL2 registers with the values from the untrusted
host memory? By copying host_vcpu->arch.ctxt into the hyp_vcpu on entry,
could the host arbitrarily modify a protected VM's state before execution?

> @@ -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 also a pre-existing issue, but leaking the protected guest's
register state into the untrusted host memory seems to violate pKVM's
confidentiality guarantees. The untrusted host EL1 can read all private EL2
registers via its mapped kvm_vcpu structure. Should this copy be avoided for
protected VMs?

[Severity: High]
By removing the assignment that copies hcr_el2 back to the host, do we break
virtual SError injection?

When the guest takes a virtual SError, the hardware clears the VSE bit in
the hyp vCPU's hcr_el2. Because we no longer sync this back in
sync_hyp_vcpu(), the host's copy remains set. On the next entry,
flush_hyp_vcpu() will read the stale host HCR_VSE bit and set it again,
which seems like it would trap the guest in an infinite virtual SError
storm.

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