[PATCH 0/2] KVM: arm64: nv: Fix sysreg RESx-ication

[PATCH 0/2] KVM: arm64: nv: Fix sysreg RESx-ication

[Marc Zyngier]

Joey recently reported that some rather basic tests were failing on
NV, and managed to track it down to critical register fields (such as
HCR_EL2.E2H) not having their expect value.

Further investigation has outlined a couple of critical issues:

- Evaluating HCR_EL2.E2H must always be done with a sanitising
  accessor, no ifs, no buts. Given that KVM assumes a fixed value for
  this bit, we cannot leave it to the guest to mess with.

- Resetting the sysreg file must result in the RESx bits taking
  effect. Otherwise, we may end-up making the wrong decision (see
  above), and we definitely expose invalid values to the guest. Note
  that because we compute the RESx masks very late in the VM setup, we
  need to apply these masks at that particular point as well.

The two patches in this series are enough to fix the current set of
issues, but __vcpu_sys_reg() needs some extra work as it is doing the
wrong thing when used as a lvalue. I'll post a separate series for
that, as the two problems are fairly orthogonal, and this results in a
significant amount of churn.

All kudos to Joey for patiently tracking that one down. This was
hidden behind a myriad of other issues, and nailing this sucker down
is nothing short of a debugging lesson. Drinks on me next time.

Unless someone shouts, I'll take this in for 6.14.

Marc Zyngier (2):
  KVM: arm64: nv: Always evaluate HCR_EL2 using sanitising accessors
  KVM: arm64: nv: Apply RESx settings to sysreg reset values

 arch/arm64/include/asm/kvm_emulate.h | 36 ++++++++++++----------------
 arch/arm64/include/asm/kvm_nested.h  |  2 +-
 arch/arm64/kvm/hyp/vhe/sysreg-sr.c   |  4 ++--
 arch/arm64/kvm/nested.c              |  9 +++++--
 arch/arm64/kvm/sys_regs.c            |  5 +++-
 5 files changed, 29 insertions(+), 27 deletions(-)

-- 
2.39.2

[PATCH 1/2] KVM: arm64: nv: Always evaluate HCR_EL2 using sanitising accessors

[Marc Zyngier]

A lot of the NV code depends on HCR_EL2.{E2H,TGE}, and we assume
in places that at least HCR_EL2.E2H is invariant for a given guest.

However, we make a point in *not* using the sanitising accessor
that would enforce this, and are at the mercy of the guest doing
stupid things. Clearly, that's not good.

Rework the HCR_EL2 accessors to use __vcpu_sys_reg() instead,
guaranteeing that the RESx settings get applied, specially
when HCR_EL2.E2H is evaluated. This results in fewer accessors
overall.

Huge thanks to Joey who spent a long time tracking this bug down.

Reported-by: Joey Gouly <Joey.Gouly@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_emulate.h | 36 ++++++++++++----------------
 arch/arm64/kvm/hyp/vhe/sysreg-sr.c   |  4 ++--
 2 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 333c163987a90..fad4f28ed7e81 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -184,29 +184,30 @@ static inline bool vcpu_is_el2(const struct kvm_vcpu *vcpu)
 	return vcpu_is_el2_ctxt(&vcpu->arch.ctxt);
 }
 
-static inline bool __vcpu_el2_e2h_is_set(const struct kvm_cpu_context *ctxt)
+static inline bool vcpu_el2_e2h_is_set(const struct kvm_vcpu *vcpu)
 {
 	return (!cpus_have_final_cap(ARM64_HAS_HCR_NV1) ||
-		(ctxt_sys_reg(ctxt, HCR_EL2) & HCR_E2H));
+		(__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_E2H));
 }
 
-static inline bool vcpu_el2_e2h_is_set(const struct kvm_vcpu *vcpu)
+static inline bool vcpu_el2_tge_is_set(const struct kvm_vcpu *vcpu)
 {
-	return __vcpu_el2_e2h_is_set(&vcpu->arch.ctxt);
+	return ctxt_sys_reg(&vcpu->arch.ctxt, HCR_EL2) & HCR_TGE;
 }
 
-static inline bool __vcpu_el2_tge_is_set(const struct kvm_cpu_context *ctxt)
+static inline bool is_hyp_ctxt(const struct kvm_vcpu *vcpu)
 {
-	return ctxt_sys_reg(ctxt, HCR_EL2) & HCR_TGE;
-}
+	bool e2h, tge;
+	u64 hcr;
 
-static inline bool vcpu_el2_tge_is_set(const struct kvm_vcpu *vcpu)
-{
-	return __vcpu_el2_tge_is_set(&vcpu->arch.ctxt);
-}
+	if (!vcpu_has_nv(vcpu))
+		return false;
+
+	hcr = __vcpu_sys_reg(vcpu, HCR_EL2);
+
+	e2h = (hcr & HCR_E2H);
+	tge = (hcr & HCR_TGE);
 
-static inline bool __is_hyp_ctxt(const struct kvm_cpu_context *ctxt)
-{
 	/*
 	 * We are in a hypervisor context if the vcpu mode is EL2 or
 	 * E2H and TGE bits are set. The latter means we are in the user space
@@ -215,14 +216,7 @@ static inline bool __is_hyp_ctxt(const struct kvm_cpu_context *ctxt)
 	 * Note that the HCR_EL2.{E2H,TGE}={0,1} isn't really handled in the
 	 * rest of the KVM code, and will result in a misbehaving guest.
 	 */
-	return vcpu_is_el2_ctxt(ctxt) ||
-		(__vcpu_el2_e2h_is_set(ctxt) && __vcpu_el2_tge_is_set(ctxt)) ||
-		__vcpu_el2_tge_is_set(ctxt);
-}
-
-static inline bool is_hyp_ctxt(const struct kvm_vcpu *vcpu)
-{
-	return vcpu_has_nv(vcpu) && __is_hyp_ctxt(&vcpu->arch.ctxt);
+	return vcpu_is_el2(vcpu) || (e2h && tge) || tge;
 }
 
 static inline bool vcpu_is_host_el0(const struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
index 5f78a39053a79..90b018e06f2cb 100644
--- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
@@ -216,7 +216,7 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
 	__sysreg32_restore_state(vcpu);
 	__sysreg_restore_user_state(guest_ctxt);
 
-	if (unlikely(__is_hyp_ctxt(guest_ctxt))) {
+	if (unlikely(is_hyp_ctxt(vcpu))) {
 		__sysreg_restore_vel2_state(vcpu);
 	} else {
 		if (vcpu_has_nv(vcpu)) {
@@ -260,7 +260,7 @@ void __vcpu_put_switch_sysregs(struct kvm_vcpu *vcpu)
 
 	host_ctxt = host_data_ptr(host_ctxt);
 
-	if (unlikely(__is_hyp_ctxt(guest_ctxt)))
+	if (unlikely(is_hyp_ctxt(vcpu)))
 		__sysreg_save_vel2_state(vcpu);
 	else
 		__sysreg_save_el1_state(guest_ctxt);
-- 
2.39.2

[PATCH 2/2] KVM: arm64: nv: Apply RESx settings to sysreg reset values

[Marc Zyngier]

While we have sanitisation in place for the guest sysregs, we lack
that sanitisation out of reset. So some of the fields could be
evaluated and not reflect their RESx status, which sounds like
a very bad idea.

Apply the RESx masks to the the sysreg file in two situations:

- when going via a reset of the sysregs

- after having computed the RESx masks

Having this separate reset phase from the actual reset handling is
a bit grotty, but we need to apply this after the ID registers are
final.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_nested.h | 2 +-
 arch/arm64/kvm/nested.c             | 9 +++++++--
 arch/arm64/kvm/sys_regs.c           | 5 ++++-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
index 4792a3f1f4841..e3cd89ed94924 100644
--- a/arch/arm64/include/asm/kvm_nested.h
+++ b/arch/arm64/include/asm/kvm_nested.h
@@ -187,7 +187,7 @@ static inline bool kvm_supported_tlbi_s1e2_op(struct kvm_vcpu *vpcu, u32 instr)
 	return true;
 }
 
-int kvm_init_nv_sysregs(struct kvm *kvm);
+int kvm_init_nv_sysregs(struct kvm_vcpu *vcpu);
 
 #ifdef CONFIG_ARM64_PTR_AUTH
 bool kvm_auth_eretax(struct kvm_vcpu *vcpu, u64 *elr);
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index 071198e1ba264..169c548f72d1a 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -1568,14 +1568,15 @@ static __always_inline void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0,
 	kvm->arch.sysreg_masks->mask[i].res1 = res1;
 }
 
-int kvm_init_nv_sysregs(struct kvm *kvm)
+int kvm_init_nv_sysregs(struct kvm_vcpu *vcpu)
 {
+	struct kvm *kvm = vcpu->kvm;
 	u64 res0, res1;
 
 	lockdep_assert_held(&kvm->arch.config_lock);
 
 	if (kvm->arch.sysreg_masks)
-		return 0;
+		goto out;
 
 	kvm->arch.sysreg_masks = kzalloc(sizeof(*(kvm->arch.sysreg_masks)),
 					 GFP_KERNEL_ACCOUNT);
@@ -1906,6 +1907,10 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 	/* VNCR_EL2 */
 	set_sysreg_masks(kvm, VNCR_EL2, VNCR_EL2_RES0, VNCR_EL2_RES1);
 
+out:
+	for (enum vcpu_sysreg sr = __SANITISED_REG_START__; sr < NR_SYS_REGS; sr++)
+		(void)__vcpu_sys_reg(vcpu, sr);
+
 	return 0;
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 4a09b6ef94bb9..18bb81291c7ce 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -4576,6 +4576,9 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
 			reset_vcpu_ftr_id_reg(vcpu, r);
 		else
 			r->reset(vcpu, r);
+
+		if (r->reg >= __SANITISED_REG_START__ && r->reg < NR_SYS_REGS)
+			(void)__vcpu_sys_reg(vcpu, r->reg);
 	}
 
 	set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);
@@ -5179,7 +5182,7 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 	}
 
 	if (vcpu_has_nv(vcpu)) {
-		int ret = kvm_init_nv_sysregs(kvm);
+		int ret = kvm_init_nv_sysregs(vcpu);
 		if (ret)
 			return ret;
 	}
-- 
2.39.2

Re: [PATCH 0/2] KVM: arm64: nv: Fix sysreg RESx-ication

[Joey Gouly]

On Sun, Jan 12, 2025 at 04:50:27PM +0000, Marc Zyngier wrote:
> Joey recently reported that some rather basic tests were failing on
> NV, and managed to track it down to critical register fields (such as
> HCR_EL2.E2H) not having their expect value.
> 
> Further investigation has outlined a couple of critical issues:
> 
> - Evaluating HCR_EL2.E2H must always be done with a sanitising
>   accessor, no ifs, no buts. Given that KVM assumes a fixed value for
>   this bit, we cannot leave it to the guest to mess with.
> 
> - Resetting the sysreg file must result in the RESx bits taking
>   effect. Otherwise, we may end-up making the wrong decision (see
>   above), and we definitely expose invalid values to the guest. Note
>   that because we compute the RESx masks very late in the VM setup, we
>   need to apply these masks at that particular point as well.
> 
> The two patches in this series are enough to fix the current set of
> issues, but __vcpu_sys_reg() needs some extra work as it is doing the
> wrong thing when used as a lvalue. I'll post a separate series for
> that, as the two problems are fairly orthogonal, and this results in a
> significant amount of churn.
> 
> All kudos to Joey for patiently tracking that one down. This was
> hidden behind a myriad of other issues, and nailing this sucker down
> is nothing short of a debugging lesson. Drinks on me next time.
> 
> Unless someone shouts, I'll take this in for 6.14.
> 

Testing using the updated kvm-arm64/nv-next branch:
Tested-by: Joey Gouly <joey.gouly@arm.com>
Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks!
Joey

> Marc Zyngier (2):
>   KVM: arm64: nv: Always evaluate HCR_EL2 using sanitising accessors
>   KVM: arm64: nv: Apply RESx settings to sysreg reset values
> 
>  arch/arm64/include/asm/kvm_emulate.h | 36 ++++++++++++----------------
>  arch/arm64/include/asm/kvm_nested.h  |  2 +-
>  arch/arm64/kvm/hyp/vhe/sysreg-sr.c   |  4 ++--
>  arch/arm64/kvm/nested.c              |  9 +++++--
>  arch/arm64/kvm/sys_regs.c            |  5 +++-
>  5 files changed, 29 insertions(+), 27 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH 0/2] KVM: arm64: nv: Fix sysreg RESx-ication

[Marc Zyngier]

On Sun, 12 Jan 2025 16:50:27 +0000, Marc Zyngier wrote:
> Joey recently reported that some rather basic tests were failing on
> NV, and managed to track it down to critical register fields (such as
> HCR_EL2.E2H) not having their expect value.
> 
> Further investigation has outlined a couple of critical issues:
> 
> - Evaluating HCR_EL2.E2H must always be done with a sanitising
>   accessor, no ifs, no buts. Given that KVM assumes a fixed value for
>   this bit, we cannot leave it to the guest to mess with.
> 
> [...]

Applied to next, thanks!

[1/2] KVM: arm64: nv: Always evaluate HCR_EL2 using sanitising accessors
      commit: c139b6d1b4d27724987af5071177fb5f3d60c1e4
[2/2] KVM: arm64: nv: Apply RESx settings to sysreg reset values
      commit: 36f998de853cfad60508dfdfb41c9c40a2245f19

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.