[PATCH v1 0/4] KVM: arm64: Prevent sysreg helper parameter transposition

[PATCH v1 0/4] KVM: arm64: Prevent sysreg helper parameter transposition

[Fuad Tabba]

Some of the KVM/arm64 sysreg helper functions and macros, such as
vcpu_write_sys_reg() and __vcpu_assign_sys_reg(), are prone to parameter
transposition bugs. The 'reg'/'r' (enum vcpu_sysreg) and 'val'/'v' (u64)
can be easily swapped, as the types are not distinct enough to be caught
by the compiler.

There are a few functions and macros that have similar parameters and
behavior, e.g., vcpu_write_sys_reg(), __vcpu_assign_sys_reg(), and
__vcpu_rmw_sys_reg(). However, the ordering of the reg and value
parameters is not consitent across them [*].

Moreover, there is neither a compile time nor a runtime check that
catches these errors. This has caused at least one bug that made it
upsteam: commit 798eb5978700 ("KVM: arm64: Sync protected guest VBAR_EL1
on injecting an undef exception"), and other kernel developers have also
run into similar issues from speaking to them.

This series addresses this in two ways:

* The parameter order of vcpu_write_sys_reg() is changed from (vcpu,
  val, reg) to (vcpu, reg, val), making it consistent with similar
  functions and macros.
* Compile-time checks are added to prevent the 'reg' parameter from
  having a 'u64' type, which directly catches the transposition bug.

No functional change is intended in this series.

Based on Linux 6.18-rc3.

Cheers,
/fuad

[*] Just take look at __vcpu_write_sys_reg() in
  arch/arm64/kvm/hyp/exception.c for example:

static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
{
	if (has_vhe())
		vcpu_write_sys_reg(vcpu, val, reg);
	else
		__vcpu_assign_sys_reg(vcpu, reg, val);
}

Fuad Tabba (4):
  KVM: arm64: Switch reg and val parameter ordering in
    vcpu_write_sys_reg()
  KVM: arm64: Add compile-time type check for register in
    __vcpu_assign_sys_reg()
  KVM: arm64: Add compile-time type check to vcpu_write_sys_reg()
  KVM: arm64: Add compile-time type check for register in
    __vcpu_rmw_sys_reg()

 arch/arm64/include/asm/kvm_emulate.h |  2 +-
 arch/arm64/include/asm/kvm_host.h    | 44 ++++++++++++++++------------
 arch/arm64/kvm/at.c                  |  6 ++--
 arch/arm64/kvm/emulate-nested.c      |  4 +--
 arch/arm64/kvm/hyp/exception.c       | 14 ++++-----
 arch/arm64/kvm/hyp/vhe/switch.c      |  2 +-
 arch/arm64/kvm/inject_fault.c        | 16 +++++-----
 arch/arm64/kvm/nested.c              |  4 +--
 arch/arm64/kvm/pmu-emul.c            |  7 +++--
 arch/arm64/kvm/sys_regs.c            | 16 +++++-----
 arch/arm64/kvm/sys_regs.h            |  2 +-
 11 files changed, 63 insertions(+), 54 deletions(-)


base-commit: dcb6fa37fd7bc9c3d2b066329b0d27dedf8becaa
-- 
2.51.1.838.g19442a804e-goog

[PATCH v1 1/4] KVM: arm64: Switch reg and val parameter ordering in vcpu_write_sys_reg()

[Fuad Tabba]

The vcpu_write_sys_reg() function previously used the signature:
void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, enum vcpu_sysreg reg);

This was inconsistent with related functions and macros such as
__vcpu_assign_sys_reg() and __vcpu_rmw_sys_reg(), both of which
have the register parameter before the value.
'(vcpu, reg, ...)' ordering.

This inconsistency has been a direct source of parameter transposition
bugs in related functions. With the 'reg' and 'val' types being simple
integers, it was easy to mistake one for the other.

This patch swaps the `reg` and `val` parameters to create the logical
and consistent signature:
void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg, u64 val);

All call sites are mechanically updated to match the new signature.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h |  2 +-
 arch/arm64/include/asm/kvm_host.h    |  2 +-
 arch/arm64/kvm/at.c                  |  6 +++---
 arch/arm64/kvm/emulate-nested.c      |  4 ++--
 arch/arm64/kvm/hyp/exception.c       | 12 ++++++------
 arch/arm64/kvm/hyp/vhe/switch.c      |  2 +-
 arch/arm64/kvm/inject_fault.c        | 16 ++++++++--------
 arch/arm64/kvm/nested.c              |  4 ++--
 arch/arm64/kvm/sys_regs.c            | 16 ++++++++--------
 9 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index c9eab316398e..f82201b3e1c9 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -532,7 +532,7 @@ static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
 
 		sctlr = vcpu_read_sys_reg(vcpu, r);
 		sctlr |= SCTLR_ELx_EE;
-		vcpu_write_sys_reg(vcpu, sctlr, r);
+		vcpu_write_sys_reg(vcpu, r, sctlr);
 	}
 }
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 64302c438355..1b8a420f9add 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1167,7 +1167,7 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 	})
 
 u64 vcpu_read_sys_reg(const struct kvm_vcpu *, enum vcpu_sysreg);
-void vcpu_write_sys_reg(struct kvm_vcpu *, u64, enum vcpu_sysreg);
+void vcpu_write_sys_reg(struct kvm_vcpu *, enum vcpu_sysreg, u64);
 
 struct kvm_vm_stat {
 	struct kvm_vm_stat_generic generic;
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index be26d5aa668c..1421971349f4 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -1424,7 +1424,7 @@ void __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	    !par_check_s1_access_fault(par))
 		par = handle_at_slow(vcpu, op, vaddr);
 
-	vcpu_write_sys_reg(vcpu, par, PAR_EL1);
+	vcpu_write_sys_reg(vcpu, PAR_EL1, par);
 }
 
 void __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
@@ -1479,7 +1479,7 @@ void __kvm_at_s1e2(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	if ((par & SYS_PAR_EL1_F) && !par_check_s1_perm_fault(par))
 		par = handle_at_slow(vcpu, op, vaddr);
 
-	vcpu_write_sys_reg(vcpu, par, PAR_EL1);
+	vcpu_write_sys_reg(vcpu, PAR_EL1, par);
 }
 
 void __kvm_at_s12(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
@@ -1538,7 +1538,7 @@ void __kvm_at_s12(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 		out.esr = ESR_ELx_FSC_PERM_L(out.level & 0x3);
 
 	par = compute_par_s12(vcpu, par, &out);
-	vcpu_write_sys_reg(vcpu, par, PAR_EL1);
+	vcpu_write_sys_reg(vcpu, PAR_EL1, par);
 }
 
 /*
diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
index 834f13fb1fb7..e30a059b71e6 100644
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -2722,7 +2722,7 @@ static void kvm_inject_el2_exception(struct kvm_vcpu *vcpu, u64 esr_el2,
 	switch (type) {
 	case except_type_sync:
 		kvm_pend_exception(vcpu, EXCEPT_AA64_EL2_SYNC);
-		vcpu_write_sys_reg(vcpu, esr_el2, ESR_EL2);
+		vcpu_write_sys_reg(vcpu, ESR_EL2, esr_el2);
 		break;
 	case except_type_irq:
 		kvm_pend_exception(vcpu, EXCEPT_AA64_EL2_IRQ);
@@ -2834,7 +2834,7 @@ int kvm_inject_nested_sea(struct kvm_vcpu *vcpu, bool iabt, u64 addr)
 			     iabt ? ESR_ELx_EC_IABT_LOW : ESR_ELx_EC_DABT_LOW);
 	esr |= ESR_ELx_FSC_EXTABT | ESR_ELx_IL;
 
-	vcpu_write_sys_reg(vcpu, addr, FAR_EL2);
+	vcpu_write_sys_reg(vcpu, FAR_EL2, addr);
 
 	if (__vcpu_sys_reg(vcpu, SCTLR2_EL2) & SCTLR2_EL1_EASE)
 		return kvm_inject_nested(vcpu, esr, except_type_serror);
diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
index bef40ddb16db..a5e5eda7aba0 100644
--- a/arch/arm64/kvm/hyp/exception.c
+++ b/arch/arm64/kvm/hyp/exception.c
@@ -28,10 +28,10 @@ static inline u64 __vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
 	return __vcpu_sys_reg(vcpu, reg);
 }
 
-static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
+static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, int reg, u64 val)
 {
 	if (has_vhe())
-		vcpu_write_sys_reg(vcpu, val, reg);
+		vcpu_write_sys_reg(vcpu, reg, val);
 	else
 		__vcpu_assign_sys_reg(vcpu, reg, val);
 }
@@ -41,9 +41,9 @@ static void __vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long target_mode,
 {
 	if (has_vhe()) {
 		if (target_mode == PSR_MODE_EL1h)
-			vcpu_write_sys_reg(vcpu, val, SPSR_EL1);
+			vcpu_write_sys_reg(vcpu, SPSR_EL1, val);
 		else
-			vcpu_write_sys_reg(vcpu, val, SPSR_EL2);
+			vcpu_write_sys_reg(vcpu, SPSR_EL2, val);
 	} else {
 		__vcpu_assign_sys_reg(vcpu, SPSR_EL1, val);
 	}
@@ -103,12 +103,12 @@ static void enter_exception64(struct kvm_vcpu *vcpu, unsigned long target_mode,
 	case PSR_MODE_EL1h:
 		vbar = __vcpu_read_sys_reg(vcpu, VBAR_EL1);
 		sctlr = __vcpu_read_sys_reg(vcpu, SCTLR_EL1);
-		__vcpu_write_sys_reg(vcpu, *vcpu_pc(vcpu), ELR_EL1);
+		__vcpu_write_sys_reg(vcpu, ELR_EL1, *vcpu_pc(vcpu));
 		break;
 	case PSR_MODE_EL2h:
 		vbar = __vcpu_read_sys_reg(vcpu, VBAR_EL2);
 		sctlr = __vcpu_read_sys_reg(vcpu, SCTLR_EL2);
-		__vcpu_write_sys_reg(vcpu, *vcpu_pc(vcpu), ELR_EL2);
+		__vcpu_write_sys_reg(vcpu, ELR_EL2, *vcpu_pc(vcpu));
 		break;
 	default:
 		/* Don't do that */
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index 9984c492305a..c6731c83b174 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -449,7 +449,7 @@ static bool kvm_hyp_handle_cpacr_el1(struct kvm_vcpu *vcpu, u64 *exit_code)
 	if ((esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ) {
 		vcpu_set_reg(vcpu, rt, __vcpu_sys_reg(vcpu, CPTR_EL2));
 	} else {
-		vcpu_write_sys_reg(vcpu, vcpu_get_reg(vcpu, rt), CPTR_EL2);
+		vcpu_write_sys_reg(vcpu, CPTR_EL2, vcpu_get_reg(vcpu, rt));
 		__activate_cptr_traps(vcpu);
 	}
 
diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index dfcd66c65517..17f4b5a9635b 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c
@@ -158,8 +158,8 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr
 
 	esr |= fsc;
 
-	vcpu_write_sys_reg(vcpu, addr, exception_far_elx(vcpu));
-	vcpu_write_sys_reg(vcpu, esr, exception_esr_elx(vcpu));
+	vcpu_write_sys_reg(vcpu, exception_far_elx(vcpu), addr);
+	vcpu_write_sys_reg(vcpu, exception_esr_elx(vcpu), esr);
 }
 
 static void inject_undef64(struct kvm_vcpu *vcpu)
@@ -175,7 +175,7 @@ static void inject_undef64(struct kvm_vcpu *vcpu)
 	if (kvm_vcpu_trap_il_is32bit(vcpu))
 		esr |= ESR_ELx_IL;
 
-	vcpu_write_sys_reg(vcpu, esr, exception_esr_elx(vcpu));
+	vcpu_write_sys_reg(vcpu, exception_esr_elx(vcpu), esr);
 }
 
 #define DFSR_FSC_EXTABT_LPAE	0x10
@@ -211,15 +211,15 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt, u32 addr)
 		kvm_pend_exception(vcpu, EXCEPT_AA32_IABT);
 		far &= GENMASK(31, 0);
 		far |= (u64)addr << 32;
-		vcpu_write_sys_reg(vcpu, fsr, IFSR32_EL2);
+		vcpu_write_sys_reg(vcpu, IFSR32_EL2, fsr);
 	} else { /* !iabt */
 		kvm_pend_exception(vcpu, EXCEPT_AA32_DABT);
 		far &= GENMASK(63, 32);
 		far |= addr;
-		vcpu_write_sys_reg(vcpu, fsr, ESR_EL1);
+		vcpu_write_sys_reg(vcpu, ESR_EL1, fsr);
 	}
 
-	vcpu_write_sys_reg(vcpu, far, FAR_EL1);
+	vcpu_write_sys_reg(vcpu, FAR_EL1, far);
 }
 
 static void __kvm_inject_sea(struct kvm_vcpu *vcpu, bool iabt, u64 addr)
@@ -275,7 +275,7 @@ void kvm_inject_size_fault(struct kvm_vcpu *vcpu)
 
 	esr = vcpu_read_sys_reg(vcpu, exception_esr_elx(vcpu));
 	esr &= ~GENMASK_ULL(5, 0);
-	vcpu_write_sys_reg(vcpu, esr, exception_esr_elx(vcpu));
+	vcpu_write_sys_reg(vcpu, exception_esr_elx(vcpu), esr);
 }
 
 /**
@@ -352,7 +352,7 @@ int kvm_inject_serror_esr(struct kvm_vcpu *vcpu, u64 esr)
 	if (!serror_is_masked(vcpu)) {
 		pend_serror_exception(vcpu);
 		esr |= FIELD_PREP(ESR_ELx_EC_MASK, ESR_ELx_EC_SERROR);
-		vcpu_write_sys_reg(vcpu, esr, exception_esr_elx(vcpu));
+		vcpu_write_sys_reg(vcpu, exception_esr_elx(vcpu), esr);
 		return 1;
 	}
 
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index f04cda40545b..277e4971b253 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -804,8 +804,8 @@ int kvm_s2_handle_perm_fault(struct kvm_vcpu *vcpu, struct kvm_s2_trans *trans)
 
 int kvm_inject_s2_fault(struct kvm_vcpu *vcpu, u64 esr_el2)
 {
-	vcpu_write_sys_reg(vcpu, vcpu->arch.fault.far_el2, FAR_EL2);
-	vcpu_write_sys_reg(vcpu, vcpu->arch.fault.hpfar_el2, HPFAR_EL2);
+	vcpu_write_sys_reg(vcpu, FAR_EL2, vcpu->arch.fault.far_el2);
+	vcpu_write_sys_reg(vcpu, HPFAR_EL2, vcpu->arch.fault.hpfar_el2);
 
 	return kvm_inject_nested_sync(vcpu, esr_el2);
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e67eb39ddc11..d0d696d0819a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -338,7 +338,7 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
 	return __vcpu_sys_reg(vcpu, reg);
 }
 
-void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, enum vcpu_sysreg reg)
+void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg, u64 val)
 {
 	struct sr_loc loc = {};
 
@@ -489,7 +489,7 @@ static bool access_rw(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *r)
 {
 	if (p->is_write)
-		vcpu_write_sys_reg(vcpu, p->regval, r->reg);
+		vcpu_write_sys_reg(vcpu, r->reg, p->regval);
 	else
 		p->regval = vcpu_read_sys_reg(vcpu, r->reg);
 
@@ -572,7 +572,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
 	}
 
 	val |= (p->regval & (mask >> shift)) << shift;
-	vcpu_write_sys_reg(vcpu, val, r->reg);
+	vcpu_write_sys_reg(vcpu, r->reg, val);
 
 	kvm_toggle_cache(vcpu, was_enabled);
 	return true;
@@ -866,14 +866,14 @@ static u64 reset_dbg_wb_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd
 static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 amair = read_sysreg(amair_el1);
-	vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1);
+	vcpu_write_sys_reg(vcpu, AMAIR_EL1, amair);
 	return amair;
 }
 
 static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 actlr = read_sysreg(actlr_el1);
-	vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1);
+	vcpu_write_sys_reg(vcpu, ACTLR_EL1, actlr);
 	return actlr;
 }
 
@@ -892,7 +892,7 @@ static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
 	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
 	mpidr |= (1ULL << 31);
-	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
+	vcpu_write_sys_reg(vcpu, MPIDR_EL1, mpidr);
 
 	return mpidr;
 }
@@ -2465,7 +2465,7 @@ static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	int reg = r->reg;
 
 	if (p->is_write)
-		vcpu_write_sys_reg(vcpu, p->regval, reg);
+		vcpu_write_sys_reg(vcpu, reg, p->regval);
 	else
 		p->regval = vcpu_read_sys_reg(vcpu, reg);
 	return true;
@@ -2656,7 +2656,7 @@ static bool access_elr(struct kvm_vcpu *vcpu,
 		       const struct sys_reg_desc *r)
 {
 	if (p->is_write)
-		vcpu_write_sys_reg(vcpu, p->regval, ELR_EL1);
+		vcpu_write_sys_reg(vcpu, ELR_EL1, p->regval);
 	else
 		p->regval = vcpu_read_sys_reg(vcpu, ELR_EL1);
 
-- 
2.51.1.838.g19442a804e-goog

[PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Fuad Tabba]

The 'r' (register) and 'val' (value) parameters in
__vcpu_assign_sys_reg() can be easily transposed. The register ID is an
enum, whereas the value is a u64. This has led to bugs in the past, and
the risk was increased by the historically inconsistent parameter
ordering of the vcpu_write_sys_reg() function.

To prevent this class of bugs, add a compile-time type compatibility check
to prevent the 'r' parameter from having a 'u64' type.

This directly catches the erroneous transposition (passing the 'u64'
value as 'r') while remaining flexible, as it does not force 'r' to
be a specific enum type.

This patch also updates several local variables in
arch/arm64/kvm/pmu-emul.c from 'u64' to 'enum vcpu_sysreg' to fix
build failures caused by the new check.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
NOTE: In practice, 'enum vcpu_sysreg' cannot grow to become compatabile
with u64 since we are limited by the size of arrays that index sysregs.
If it does grow, other compile-time issues show up before this check
becomes an issue.

I verified that this would catch the bug that commit 798eb5978700
("KVM: arm64: Sync protected guest VBAR_EL1 on injecting an undef
exception") introduced, which was fixed later.
---
 arch/arm64/include/asm/kvm_host.h | 17 +++++++++--------
 arch/arm64/kvm/pmu-emul.c         |  7 ++++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1b8a420f9add..2c33ea5fdb1c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1136,14 +1136,15 @@ static inline u64 *___ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
 
 u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 
-#define __vcpu_assign_sys_reg(v, r, val)				\
-	do {								\
-		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
-		u64 __v = (val);					\
-		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
-			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
-									\
-		ctxt_sys_reg(ctxt, (r)) = __v;				\
+#define __vcpu_assign_sys_reg(v, r, val)					\
+	do {									\
+		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;		\
+		u64 __v = (val);						\
+		BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(r), u64));\
+		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)		\
+			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);		\
+										\
+		ctxt_sys_reg(ctxt, (r)) = __v;					\
 	} while (0)
 
 #define __vcpu_rmw_sys_reg(v, r, op, val)				\
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index b03dbda7f1ab..c7e2d9be77ae 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -160,7 +160,7 @@ u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
 static void kvm_pmu_set_pmc_value(struct kvm_pmc *pmc, u64 val, bool force)
 {
 	struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
-	u64 reg;
+	enum vcpu_sysreg reg;
 
 	kvm_pmu_release_perf_event(pmc);
 
@@ -230,7 +230,8 @@ static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
 static void kvm_pmu_stop_counter(struct kvm_pmc *pmc)
 {
 	struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
-	u64 reg, val;
+	enum vcpu_sysreg reg;
+	u64 val;
 
 	if (!pmc->perf_event)
 		return;
@@ -776,7 +777,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
 				    u64 select_idx)
 {
 	struct kvm_pmc *pmc = kvm_vcpu_idx_to_pmc(vcpu, select_idx);
-	u64 reg;
+	enum vcpu_sysreg reg;
 
 	reg = counter_index_to_evtreg(pmc->idx);
 	__vcpu_assign_sys_reg(vcpu, reg, (data & kvm_pmu_evtyper_mask(vcpu->kvm)));
-- 
2.51.1.838.g19442a804e-goog

[PATCH v1 3/4] KVM: arm64: Add compile-time type check to vcpu_write_sys_reg()

[Fuad Tabba]

The vcpu_write_sys_reg() function, like __vcpu_assign_sys_reg(),
is susceptible to parameter transposition bugs where the 'u64 val' is
accidentally passed in the 'reg' slot.

Even with the now-consistent parameter ordering, this remains a risk.

To apply the same compile-time checks, vcpu_write_sys_reg() is converted
into a macro. The original function implementation is converted into a
helper as _vcpu_write_sys_reg(). The new macro wrapper performs the same
type compatibility check as its `__vcpu_assign_sys_reg` counterpart,
preventing the 'reg' parameter from being a 'u64'.

This patch also includes related cleanups made necessary or apparent by
this change:
* The 'reg' parameters for __vcpu_read_sys_reg() and
  __vcpu_write_sys_reg() are tightened from 'int' to 'enum vcpu_sysreg'.
* The 'struct sys_reg_desc.reg' field is changed from 'int' to 'enum
  vcpu_sysreg'.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_host.h | 8 +++++++-
 arch/arm64/kvm/hyp/exception.c    | 4 ++--
 arch/arm64/kvm/sys_regs.c         | 2 +-
 arch/arm64/kvm/sys_regs.h         | 2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2c33ea5fdb1c..2b7c8ba8802d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1168,7 +1168,13 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 	})
 
 u64 vcpu_read_sys_reg(const struct kvm_vcpu *, enum vcpu_sysreg);
-void vcpu_write_sys_reg(struct kvm_vcpu *, enum vcpu_sysreg, u64);
+void _vcpu_write_sys_reg(struct kvm_vcpu *, enum vcpu_sysreg, u64);
+
+#define vcpu_write_sys_reg(v, r, val)						\
+	do {									\
+		BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(r), u64));\
+		_vcpu_write_sys_reg(v, r, val);					\
+	} while (0)
 
 struct kvm_vm_stat {
 	struct kvm_vm_stat_generic generic;
diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
index a5e5eda7aba0..6f8b1b5f2cab 100644
--- a/arch/arm64/kvm/hyp/exception.c
+++ b/arch/arm64/kvm/hyp/exception.c
@@ -20,7 +20,7 @@
 #error Hypervisor code only!
 #endif
 
-static inline u64 __vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
+static inline u64 __vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
 {
 	if (has_vhe())
 		return vcpu_read_sys_reg(vcpu, reg);
@@ -28,7 +28,7 @@ static inline u64 __vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
 	return __vcpu_sys_reg(vcpu, reg);
 }
 
-static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, int reg, u64 val)
+static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg, u64 val)
 {
 	if (has_vhe())
 		vcpu_write_sys_reg(vcpu, reg, val);
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d0d696d0819a..8e323353383c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -338,7 +338,7 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
 	return __vcpu_sys_reg(vcpu, reg);
 }
 
-void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg, u64 val)
+void _vcpu_write_sys_reg(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg, u64 val)
 {
 	struct sr_loc loc = {};
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index b3f904472fac..a98c3aadcdbd 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -78,7 +78,7 @@ struct sys_reg_desc {
 	u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
 
 	/* Index into sys_reg[], or 0 if we don't need to save it. */
-	int reg;
+	enum vcpu_sysreg reg;
 
 	/* Value (usually reset value), or write mask for idregs */
 	u64 val;
-- 
2.51.1.838.g19442a804e-goog

[PATCH v1 4/4] KVM: arm64: Add compile-time type check for register in __vcpu_rmw_sys_reg()

[Fuad Tabba]

Although it is less likely that the 'r' (register) and 'val' (value)
parameters in __vcpu_rmw_sys_reg() would be swapped, it is still
possible. The register ID is an int/enum, whereas the value is a u64, as
in other similar functions and macros. The cost of adding a compile-time
check to prevent this is minimal.

To prevent this class of bugs, add a compile-time type compatibility
check to prevent the 'r' parameter from having a 'u64' type.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_host.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2b7c8ba8802d..9758ba502ed5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1147,15 +1147,16 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 		ctxt_sys_reg(ctxt, (r)) = __v;					\
 	} while (0)
 
-#define __vcpu_rmw_sys_reg(v, r, op, val)				\
-	do {								\
-		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
-		u64 __v = ctxt_sys_reg(ctxt, (r));			\
-		__v op (val);						\
-		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
-			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
-									\
-		ctxt_sys_reg(ctxt, (r)) = __v;				\
+#define __vcpu_rmw_sys_reg(v, r, op, val)					\
+	do {									\
+		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;		\
+		u64 __v = ctxt_sys_reg(ctxt, (r));				\
+		BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(r), u64));\
+		__v op (val);							\
+		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)		\
+			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);		\
+										\
+		ctxt_sys_reg(ctxt, (r)) = __v;					\
 	} while (0)
 
 #define __vcpu_sys_reg(v,r)						\
-- 
2.51.1.838.g19442a804e-goog

Re: [PATCH v1 1/4] KVM: arm64: Switch reg and val parameter ordering in vcpu_write_sys_reg()

[Marc Zyngier]

On Mon, 27 Oct 2025 11:39:40 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> The vcpu_write_sys_reg() function previously used the signature:
> void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, enum vcpu_sysreg reg);
> 
> This was inconsistent with related functions and macros such as
> __vcpu_assign_sys_reg() and __vcpu_rmw_sys_reg(), both of which
> have the register parameter before the value.
> '(vcpu, reg, ...)' ordering.

On the other hand, write_sys_reg() is exactly in the same order as
vcpu_write_sys_reg(). I think that's the model to follow.

The __vcpu_*_sys_reg() helpers are internal things, really, and if
something has to change, I'd rather change those.

Thanks,

	M.

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

Re: [PATCH v1 1/4] KVM: arm64: Switch reg and val parameter ordering in vcpu_write_sys_reg()

[Fuad Tabba]

Hi Marc,

On Tue, 28 Oct 2025 at 10:38, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 27 Oct 2025 11:39:40 +0000,
> Fuad Tabba <tabba@google.com> wrote:
> >
> > The vcpu_write_sys_reg() function previously used the signature:
> > void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, enum vcpu_sysreg reg);
> >
> > This was inconsistent with related functions and macros such as
> > __vcpu_assign_sys_reg() and __vcpu_rmw_sys_reg(), both of which
> > have the register parameter before the value.
> > '(vcpu, reg, ...)' ordering.
>
> On the other hand, write_sys_reg() is exactly in the same order as
> vcpu_write_sys_reg(). I think that's the model to follow.
>
> The __vcpu_*_sys_reg() helpers are internal things, really, and if
> something has to change, I'd rather change those.

Sure. I'll wait a bit for additional comments on the remaining
patches, and respin changing the order for the __vcpu_*_sys_reg()
helpers.

Thanks,
/fuad

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

Re: [PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Marc Zyngier]

On Mon, 27 Oct 2025 11:39:41 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> The 'r' (register) and 'val' (value) parameters in
> __vcpu_assign_sys_reg() can be easily transposed. The register ID is an
> enum, whereas the value is a u64. This has led to bugs in the past, and
> the risk was increased by the historically inconsistent parameter
> ordering of the vcpu_write_sys_reg() function.
> 
> To prevent this class of bugs, add a compile-time type compatibility check
> to prevent the 'r' parameter from having a 'u64' type.
> 
> This directly catches the erroneous transposition (passing the 'u64'
> value as 'r') while remaining flexible, as it does not force 'r' to
> be a specific enum type.

I don't think that's enough. It is too easy to pass a u32 and
accidentally bypass this check. I really think we should redefine the
register identifiers to be a fixed value structure, something like
this:

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7501a2ee4dd44..18f65dbff2379 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -624,6 +624,12 @@ enum vcpu_sysreg {
 	NR_SYS_REGS	/* Nothing after this line! */
 };
 
+struct vcpu_sys_reg {
+	enum vcpu_sysreg reg;
+};
+
+static const struct vcpu_sys_reg vcpu_sys_reg_SCTLR_EL1 = { SCTLR_EL1 };
+
 struct kvm_sysreg_masks {
 	struct {
 		u64	res0;

and then enforce that whatever is interpreted as a system register is
actually one of these structures (with some creative cpp wrapping to
make it less awkward).

In a way, this is similar to what we are doing for page table
manipulation, where pte_t is a structure wrapping a u64.

If we move to such construct, it becomes impossible to swap value and
register, and we can stop worrying about that.

Thoughts?

	M.

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

Re: [PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Fuad Tabba]

Hi Marc,

On Tue, 28 Oct 2025 at 10:51, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 27 Oct 2025 11:39:41 +0000,
> Fuad Tabba <tabba@google.com> wrote:
> >
> > The 'r' (register) and 'val' (value) parameters in
> > __vcpu_assign_sys_reg() can be easily transposed. The register ID is an
> > enum, whereas the value is a u64. This has led to bugs in the past, and
> > the risk was increased by the historically inconsistent parameter
> > ordering of the vcpu_write_sys_reg() function.
> >
> > To prevent this class of bugs, add a compile-time type compatibility check
> > to prevent the 'r' parameter from having a 'u64' type.
> >
> > This directly catches the erroneous transposition (passing the 'u64'
> > value as 'r') while remaining flexible, as it does not force 'r' to
> > be a specific enum type.
>
> I don't think that's enough. It is too easy to pass a u32 and
> accidentally bypass this check. I really think we should redefine the
> register identifiers to be a fixed value structure, something like
> this:
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7501a2ee4dd44..18f65dbff2379 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -624,6 +624,12 @@ enum vcpu_sysreg {
>         NR_SYS_REGS     /* Nothing after this line! */
>  };
>
> +struct vcpu_sys_reg {
> +       enum vcpu_sysreg reg;
> +};
> +
> +static const struct vcpu_sys_reg vcpu_sys_reg_SCTLR_EL1 = { SCTLR_EL1 };
> +
>  struct kvm_sysreg_masks {
>         struct {
>                 u64     res0;
>
> and then enforce that whatever is interpreted as a system register is
> actually one of these structures (with some creative cpp wrapping to
> make it less awkward).
>
> In a way, this is similar to what we are doing for page table
> manipulation, where pte_t is a structure wrapping a u64.
>
> If we move to such construct, it becomes impossible to swap value and
> register, and we can stop worrying about that.
>
> Thoughts?

I considered that, but then went with the approach I have here to
reduce code churn. I do prefer wrapping it in a struct, so I'll do
that when I respin.

Thanks,
/fuad

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

Re: [PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Fuad Tabba]

Hi Marc,

On Tue, 28 Oct 2025 at 10:51, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 27 Oct 2025 11:39:41 +0000,
> Fuad Tabba <tabba@google.com> wrote:
> >
> > The 'r' (register) and 'val' (value) parameters in
> > __vcpu_assign_sys_reg() can be easily transposed. The register ID is an
> > enum, whereas the value is a u64. This has led to bugs in the past, and
> > the risk was increased by the historically inconsistent parameter
> > ordering of the vcpu_write_sys_reg() function.
> >
> > To prevent this class of bugs, add a compile-time type compatibility check
> > to prevent the 'r' parameter from having a 'u64' type.
> >
> > This directly catches the erroneous transposition (passing the 'u64'
> > value as 'r') while remaining flexible, as it does not force 'r' to
> > be a specific enum type.
>
> I don't think that's enough. It is too easy to pass a u32 and
> accidentally bypass this check. I really think we should redefine the
> register identifiers to be a fixed value structure, something like
> this:
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7501a2ee4dd44..18f65dbff2379 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -624,6 +624,12 @@ enum vcpu_sysreg {
>         NR_SYS_REGS     /* Nothing after this line! */
>  };
>
> +struct vcpu_sys_reg {
> +       enum vcpu_sysreg reg;
> +};
> +
> +static const struct vcpu_sys_reg vcpu_sys_reg_SCTLR_EL1 = { SCTLR_EL1 };
> +
>  struct kvm_sysreg_masks {
>         struct {
>                 u64     res0;
>
> and then enforce that whatever is interpreted as a system register is
> actually one of these structures (with some creative cpp wrapping to
> make it less awkward).
>
> In a way, this is similar to what we are doing for page table
> manipulation, where pte_t is a structure wrapping a u64.
>
> If we move to such construct, it becomes impossible to swap value and
> register, and we can stop worrying about that.
>
> Thoughts?

I looked into this some more, and the issue is that the number of
callers who refer to members of `enum vcpu_sysreg` is really big, as
well as the functions that access its value (in if or switch
statements). It can be done, but the amount of code churn would be
huge, that in practice I don't think would be practical.

Another check I thought of, which is more robust and would catch all
cases we have encountered in practice is something along the lines of:

BUILD_BUG_ON(!__builtin_types_compatible_p(typeof(reg), enum vcpu_sysreg) &&
    !__builtin_types_compatible_p(typeof(reg), int));

This ensures that you cannot pass anything other than an enum value,
or an int, as a sysreg. U32 and U64 would trigger a build error. I
tested this, no callers needed to be changed and it did catch the bug
I am trying to avoid. It's not as robust as wrapping in a struct, but
it doesn't result in any churn.

What do you think?
/fuad


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

Re: [PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Fuad Tabba]

Hi Marc,

On Tue, 28 Oct 2025 at 10:51, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 27 Oct 2025 11:39:41 +0000,
> Fuad Tabba <tabba@google.com> wrote:
> >
> > The 'r' (register) and 'val' (value) parameters in
> > __vcpu_assign_sys_reg() can be easily transposed. The register ID is an
> > enum, whereas the value is a u64. This has led to bugs in the past, and
> > the risk was increased by the historically inconsistent parameter
> > ordering of the vcpu_write_sys_reg() function.
> >
> > To prevent this class of bugs, add a compile-time type compatibility check
> > to prevent the 'r' parameter from having a 'u64' type.
> >
> > This directly catches the erroneous transposition (passing the 'u64'
> > value as 'r') while remaining flexible, as it does not force 'r' to
> > be a specific enum type.
>
> I don't think that's enough. It is too easy to pass a u32 and
> accidentally bypass this check. I really think we should redefine the
> register identifiers to be a fixed value structure, something like
> this:
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7501a2ee4dd44..18f65dbff2379 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -624,6 +624,12 @@ enum vcpu_sysreg {
>         NR_SYS_REGS     /* Nothing after this line! */
>  };
>
> +struct vcpu_sys_reg {
> +       enum vcpu_sysreg reg;
> +};
> +
> +static const struct vcpu_sys_reg vcpu_sys_reg_SCTLR_EL1 = { SCTLR_EL1 };
> +
>  struct kvm_sysreg_masks {
>         struct {
>                 u64     res0;
>
> and then enforce that whatever is interpreted as a system register is
> actually one of these structures (with some creative cpp wrapping to
> make it less awkward).

I thought about this some more, after our conversation yesterday, and
I still don't see how I can do it without having to change most
callsites _and_ getting strong type checking. This is of course
assuming I understood you correctly :D

For example, the most common parameter for __vcpu_assign_sys_reg() is
an `enum vcpu_sysreg`, e.g., __vcpu_assign_sys_reg(vcpu, PAR_EL1,
val);

If we wrap it as follows (after renaming __vcpu_assign_sys_reg to
___vcpu_assign_sys_reg), it still doesn't add any type checking:

define __vcpu_assign_sys_reg(vcpu, reg, val) \
do { \
   const struct vcpu_sys_reg vcpu_sys_reg ## reg = { reg }; \
    ___vcpu_assign_sys_reg(vcpu, vcpu_sys_reg, val); \
} while (0)

I can't think of a way to do it other than making sure that SCTLR_EL1
is from the beginning a `struct vcpu_sys_reg` type rather than an
`enum vcpu_sysreg` type, which means either doing a __pte(x)-like
thing at all call sites .

An alternative would be to construct a list of `static const struct
vcpu_sys_reg` for all enums, which is what you might have been
referring to above. If that's the case, what is the best way to do it?
XMacros, that would be tricky especially with the VNCR() within the
enum. Or pregenerate with a script, which seems to be too much effort
for this.

Like I mentioned, even though it's not foolproof, in practice, something like

BUILD_BUG_ON(!__builtin_types_compatible_p(typeof(reg), enum
vcpu_sysreg) && !__builtin_types_compatible_p(typeof(reg), int));

If I understand the C standard correctly, enumeration constants
regardless of the size of the enum itself are always an int.
Therefore, this check is correct, even if the enum expands beyond u32
(which is very unlikely anyway), but might miss some cases if we
happen to transpose a `val` that is int with `reg` . That said, I
think that in practice it catches all the transposition cases we've
encountered.

What do you think?

Thanks,
/fuad





>
> In a way, this is similar to what we are doing for page table
> manipulation, where pte_t is a structure wrapping a u64.
>
> If we move to such construct, it becomes impossible to swap value and
> register, and we can stop worrying about that.
>
> Thoughts?
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.