Re: [PATCH v1 05/11] KVM: arm64: Make vcpu_{read,write}_sys_reg available to HYP code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware/Memory State Desynchronization in EL2 sysreg accessors
--

commit 69d4753f169931c6c1990fc9118b5a6cffe54554
Author: tabba@google.com <tabba@google.com>

KVM: arm64: Make vcpu_{read,write}_sys_reg available to HYP code

The vcpu_{read,write}_sys_reg() accessors are host-only, so helpers
built on them such as kvm_vcpu_set_be()/kvm_vcpu_is_be() cannot be
shared with hyp code. Add _vcpu_read_sys_reg()/_vcpu_write_sys_reg()
inlines in kvm_emulate.h that dispatch on is_nvhe_hyp_code() to the
host- or hyp-side accessor. A follow-up series uses this to share that
emulation code at EL2.

> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 5bf3d7e1d92c7..aed9fc0b717b1 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -506,6 +506,22 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
>  	return __vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
>  }
>  
> +static inline u64 _vcpu_read_sys_reg(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
> +{
> +	if (!is_nvhe_hyp_code())
> +		return vcpu_read_sys_reg(vcpu, reg);
> +
> +	return __vcpu_sys_reg(vcpu, reg);
> +}
> +
> +static inline void _vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, enum vcpu_sysreg reg)
> +{
> +	if (!is_nvhe_hyp_code())
> +		vcpu_write_sys_reg(vcpu, val, reg);
> +	else
> +		__vcpu_assign_sys_reg(vcpu, reg, val);
> +}

[Severity: High]
If these accessors are called from nVHE EL2 code during guest exit handling,
will they read and write from the in-memory sys_regs array instead of the
active hardware registers?

Since the guest's system registers remain live in the CPU hardware during the
run loop:

    do {
        __guest_enter(vcpu);
    } while (fixup_guest_exit(vcpu, &exit_code));

and aren't saved to memory until __sysreg_save_state_nvhe() runs after the
loop terminates, it seems _vcpu_read_sys_reg() might read stale values from
memory.

Likewise, would _vcpu_write_sys_reg() updates write to memory without updating
the active hardware registers before the guest is re-entered?

> +
>  static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
>  {
>  	if (vcpu_mode_is_32bit(vcpu)) {
> @@ -516,9 +532,9 @@ static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
>  
>  		r = vcpu_has_nv(vcpu) ? SCTLR_EL2 : SCTLR_EL1;
>  
> -		sctlr = vcpu_read_sys_reg(vcpu, r);
> +		sctlr = _vcpu_read_sys_reg(vcpu, r);
>  		sctlr |= SCTLR_ELx_EE;
> -		vcpu_write_sys_reg(vcpu, sctlr, r);
> +		_vcpu_write_sys_reg(vcpu, sctlr, r);
>  	}
>  }

[Severity: High]
When kvm_vcpu_set_be() is used at EL2, could this result in the guest being
re-entered without the new endianness taking effect?

Whether updating the 32-bit pstate via *vcpu_cpsr() or the 64-bit SCTLR via
_vcpu_write_sys_reg(), the modifications appear to happen to the in-memory
state rather than the active hardware registers (like SPSR_EL2 or SCTLR_EL1).

If so, wouldn't the updates be lost and unobserved by the guest upon re-entry?

>  
> @@ -533,7 +549,7 @@ static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu)
>  	r = is_hyp_ctxt(vcpu) ? SCTLR_EL2 : SCTLR_EL1;
>  	bit = vcpu_mode_priv(vcpu) ? SCTLR_ELx_EE : SCTLR_EL1_E0E;
>  
> -	return vcpu_read_sys_reg(vcpu, r) & bit;
> +	return _vcpu_read_sys_reg(vcpu, r) & bit;
>  }

[Severity: High]
Could this result in kvm_vcpu_is_be() reading an incorrect endianness state
at EL2?

If the guest modifies SCTLR_EL1 and traps to EL2, calling this helper would
use _vcpu_read_sys_reg() which accesses the stale memory array instead of the
current hardware register state.

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