[PATCH 0/3] KVM: arm64: BUG FIX: Correctly handle zero register transfers

[PATCH 0/3] KVM: arm64: BUG FIX: Correctly handle zero register transfers

[Pavel Fedin]

ARM64 CPU has zero register which is read-only, with a value of 0.
However, KVM currently incorrectly recognizes it being SP (because
Rt == 31, and in struct user_pt_regs 'regs' array is followed by SP),
resulting in invalid value being read, or even SP corruption on write.

The problem has been discovered by performing an operation

 *((volatile int *)reg) = 0;

which compiles as "str xzr, [xx]", and resulted in strange values being
written.

Pavel Fedin (3):
  KVM: arm64: Correctly handle zero register during MMIO
  KVM: arm64: Correctly handle zero register in system register accesses
  KVM: arm64: Get rid of old vcpu_reg()

 arch/arm/include/asm/kvm_emulate.h   | 12 ++++++
 arch/arm/kvm/mmio.c                  |  5 ++-
 arch/arm/kvm/psci.c                  | 20 ++++-----
 arch/arm64/include/asm/kvm_emulate.h | 18 +++++---
 arch/arm64/kvm/handle_exit.c         |  2 +-
 arch/arm64/kvm/sys_regs.c            | 79 ++++++++++++++++++++----------------
 arch/arm64/kvm/sys_regs.h            |  4 +-
 arch/arm64/kvm/sys_regs_generic_v8.c |  2 +-
 8 files changed, 85 insertions(+), 57 deletions(-)

-- 
2.4.4

[PATCH 1/3] KVM: arm64: Correctly handle zero register during MMIO

[Pavel Fedin]

On ARM64 register index of 31 corresponds to both zero register and SP.
However, all memory access instructions, use ZR as transfer register. SP
is used only as a base register in indirect memory addressing, or by
register-register arithmetics, which cannot be trapped here.

Correct emulation is achieved by introducing new register accessor
functions, which can do special handling for reg_num == 31. These new
accessors intentionally do not rely on old vcpu_reg() on ARM64, because
it is to be removed. Since the affected code is shared by both ARM
flavours, implementations of these accessors are also added to ARM32 code.

This patch fixes setting MMIO register to a random value (actually SP)
instead of zero by something like:

 *((volatile int *)reg) = 0;

compilers tend to generate "str wzr, [xx]" here

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/include/asm/kvm_emulate.h   | 12 ++++++++++++
 arch/arm/kvm/mmio.c                  |  5 +++--
 arch/arm64/include/asm/kvm_emulate.h | 13 +++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index a9c80a2..b7ff32e 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -28,6 +28,18 @@
 unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
 unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
 
+static inline unsigned long vcpu_get_reg(const struct kvm_vcpu *vcpu,
+					 u8 reg_num)
+{
+	return *vcpu_reg(vcpu, reg_num);
+}
+
+static inline void vcpu_set_reg(const struct kvm_vcpu *vcpu, u8 reg_num,
+				unsigned long val)
+{
+	*vcpu_reg(vcpu, reg_num) = val;
+}
+
 bool kvm_condition_valid(struct kvm_vcpu *vcpu);
 void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr);
 void kvm_inject_undefined(struct kvm_vcpu *vcpu);
diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 974b1c6..3a10c9f 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -115,7 +115,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
 			       data);
 		data = vcpu_data_host_to_guest(vcpu, data, len);
-		*vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data;
+		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
 	}
 
 	return 0;
@@ -186,7 +186,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	rt = vcpu->arch.mmio_decode.rt;
 
 	if (is_write) {
-		data = vcpu_data_guest_to_host(vcpu, *vcpu_reg(vcpu, rt), len);
+		data = vcpu_data_guest_to_host(vcpu, vcpu_get_reg(vcpu, rt),
+					       len);
 
 		trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, len, fault_ipa, data);
 		mmio_write_buf(data_buf, len, data);
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 3ca894e..5a182af 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -109,6 +109,19 @@ static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
 	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num];
 }
 
+static inline unsigned long vcpu_get_reg(const struct kvm_vcpu *vcpu,
+					 u8 reg_num)
+{
+	return (reg_num == 31) ? 0 : vcpu_gp_regs(vcpu)->regs.regs[reg_num];
+}
+
+static inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num,
+				unsigned long val)
+{
+	if (reg_num != 31)
+		vcpu_gp_regs(vcpu)->regs.regs[reg_num] = val;
+}
+
 /* Get vcpu SPSR for current mode */
 static inline unsigned long *vcpu_spsr(const struct kvm_vcpu *vcpu)
 {
-- 
2.4.4

[PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Pavel Fedin]

System register accesses also use zero register for Rt == 31, and
therefore using it will also result in getting SP value instead. This
patch makes them also using new accessors, introduced by the previous
patch.

Additionally, got rid of "massive hack" in kvm_handle_cp_64().

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm64/kvm/sys_regs.c            | 79 ++++++++++++++++++++----------------
 arch/arm64/kvm/sys_regs.h            |  4 +-
 arch/arm64/kvm/sys_regs_generic_v8.c |  2 +-
 3 files changed, 46 insertions(+), 39 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 87a64e8..a667228 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
 
 	BUG_ON(!p->is_write);
 
-	val = *vcpu_reg(vcpu, p->Rt);
+	val = *p->val;
 	if (!p->is_aarch32) {
 		vcpu_sys_reg(vcpu, r->reg) = val;
 	} else {
@@ -125,13 +125,10 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 			   const struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
-	u64 val;
-
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p);
 
-	val = *vcpu_reg(vcpu, p->Rt);
-	vgic_v3_dispatch_sgi(vcpu, val);
+	vgic_v3_dispatch_sgi(vcpu, *p->val);
 
 	return true;
 }
@@ -153,7 +150,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 	if (p->is_write) {
 		return ignore_write(vcpu, p);
 	} else {
-		*vcpu_reg(vcpu, p->Rt) = (1 << 3);
+		*p->val = (1 << 3);
 		return true;
 	}
 }
@@ -167,7 +164,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
 	} else {
 		u32 val;
 		asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
-		*vcpu_reg(vcpu, p->Rt) = val;
+		*p->val = val;
 		return true;
 	}
 }
@@ -204,13 +201,13 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
 			    const struct sys_reg_desc *r)
 {
 	if (p->is_write) {
-		vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
+		vcpu_sys_reg(vcpu, r->reg) = *p->val;
 		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
 	} else {
-		*vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg);
+		*p->val = vcpu_sys_reg(vcpu, r->reg);
 	}
 
-	trace_trap_reg(__func__, r->reg, p->is_write, *vcpu_reg(vcpu, p->Rt));
+	trace_trap_reg(__func__, r->reg, p->is_write, *p->val);
 
 	return true;
 }
@@ -228,7 +225,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
 			      const struct sys_reg_params *p,
 			      u64 *dbg_reg)
 {
-	u64 val = *vcpu_reg(vcpu, p->Rt);
+	u64 val = *p->val;
 
 	if (p->is_32bit) {
 		val &= 0xffffffffUL;
@@ -248,7 +245,7 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
 	if (p->is_32bit)
 		val &= 0xffffffffUL;
 
-	*vcpu_reg(vcpu, p->Rt) = val;
+	*p->val = val;
 }
 
 static inline bool trap_bvr(struct kvm_vcpu *vcpu,
@@ -697,10 +694,10 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
 		u64 pfr = read_system_reg(SYS_ID_AA64PFR0_EL1);
 		u32 el3 = !!cpuid_feature_extract_field(pfr, ID_AA64PFR0_EL3_SHIFT);
 
-		*vcpu_reg(vcpu, p->Rt) = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
-					  (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
-					  (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20) |
-					  (6 << 16) | (el3 << 14) | (el3 << 12));
+		*p->val = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
+			   (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
+			   (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20) |
+			   (6 << 16) | (el3 << 14) | (el3 << 12));
 		return true;
 	}
 }
@@ -710,10 +707,10 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
 			 const struct sys_reg_desc *r)
 {
 	if (p->is_write) {
-		vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
+		vcpu_cp14(vcpu, r->reg) = *p->val;
 		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
 	} else {
-		*vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
+		*p->val = vcpu_cp14(vcpu, r->reg);
 	}
 
 	return true;
@@ -740,12 +737,12 @@ static inline bool trap_xvr(struct kvm_vcpu *vcpu,
 		u64 val = *dbg_reg;
 
 		val &= 0xffffffffUL;
-		val |= *vcpu_reg(vcpu, p->Rt) << 32;
+		val |= *p->val << 32;
 		*dbg_reg = val;
 
 		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
 	} else {
-		*vcpu_reg(vcpu, p->Rt) = *dbg_reg >> 32;
+		*p->val = *dbg_reg >> 32;
 	}
 
 	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
@@ -1062,12 +1059,14 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 {
 	struct sys_reg_params params;
 	u32 hsr = kvm_vcpu_get_hsr(vcpu);
+	int Rt = (hsr >> 5) & 0xf;
 	int Rt2 = (hsr >> 10) & 0xf;
+	unsigned long val;
 
 	params.is_aarch32 = true;
 	params.is_32bit = false;
 	params.CRm = (hsr >> 1) & 0xf;
-	params.Rt = (hsr >> 5) & 0xf;
+	params.val = &val;
 	params.is_write = ((hsr & 1) == 0);
 
 	params.Op0 = 0;
@@ -1076,15 +1075,12 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	params.CRn = 0;
 
 	/*
-	 * Massive hack here. Store Rt2 in the top 32bits so we only
-	 * have one register to deal with. As we use the same trap
+	 * Make a 64-bit value out of Rt and Rt2. As we use the same trap
 	 * backends between AArch32 and AArch64, we get away with it.
 	 */
 	if (params.is_write) {
-		u64 val = *vcpu_reg(vcpu, params.Rt);
-		val &= 0xffffffff;
-		val |= *vcpu_reg(vcpu, Rt2) << 32;
-		*vcpu_reg(vcpu, params.Rt) = val;
+		val = vcpu_get_reg(vcpu, Rt) & 0xffffffff;
+		val |= vcpu_get_reg(vcpu, Rt2) << 32;
 	}
 
 	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
@@ -1095,11 +1091,10 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	unhandled_cp_access(vcpu, &params);
 
 out:
-	/* Do the opposite hack for the read side */
+	/* Split up the value between registers for the read side */
 	if (!params.is_write) {
-		u64 val = *vcpu_reg(vcpu, params.Rt);
-		val >>= 32;
-		*vcpu_reg(vcpu, Rt2) = val;
+		vcpu_set_reg(vcpu, Rt, val & 0xffffffff);
+		vcpu_set_reg(vcpu, Rt, val >> 32);
 	}
 
 	return 1;
@@ -1118,21 +1113,27 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 {
 	struct sys_reg_params params;
 	u32 hsr = kvm_vcpu_get_hsr(vcpu);
+	int Rt  = (hsr >> 5) & 0xf;
+	unsigned long val = vcpu_get_reg(vcpu, Rt);
 
 	params.is_aarch32 = true;
 	params.is_32bit = true;
 	params.CRm = (hsr >> 1) & 0xf;
-	params.Rt  = (hsr >> 5) & 0xf;
+	params.val = &val;
 	params.is_write = ((hsr & 1) == 0);
 	params.CRn = (hsr >> 10) & 0xf;
 	params.Op0 = 0;
 	params.Op1 = (hsr >> 14) & 0x7;
 	params.Op2 = (hsr >> 17) & 0x7;
 
-	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
+	if (!emulate_cp(vcpu, &params, target_specific, nr_specific)) {
+		vcpu_set_reg(vcpu, Rt, val);
 		return 1;
-	if (!emulate_cp(vcpu, &params, global, nr_global))
+	}
+	if (!emulate_cp(vcpu, &params, global, nr_global)) {
+		vcpu_set_reg(vcpu, Rt, val);
 		return 1;
+	}
 
 	unhandled_cp_access(vcpu, &params);
 	return 1;
@@ -1230,6 +1231,9 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
 	struct sys_reg_params params;
 	unsigned long esr = kvm_vcpu_get_hsr(vcpu);
+	int Rt = (esr >> 5) & 0x1f;
+	unsigned long val = vcpu_get_reg(vcpu, Rt);
+	int ret;
 
 	trace_kvm_handle_sys_reg(esr);
 
@@ -1240,10 +1244,13 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	params.CRn = (esr >> 10) & 0xf;
 	params.CRm = (esr >> 1) & 0xf;
 	params.Op2 = (esr >> 17) & 0x7;
-	params.Rt = (esr >> 5) & 0x1f;
+	params.val = &val;
 	params.is_write = !(esr & 1);
 
-	return emulate_sys_reg(vcpu, &params);
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, val);
+	return ret;
 }
 
 /******************************************************************************
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index eaa324e..3267518 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -28,7 +28,7 @@ struct sys_reg_params {
 	u8	CRn;
 	u8	CRm;
 	u8	Op2;
-	u8	Rt;
+	u_long	*val;
 	bool	is_write;
 	bool	is_aarch32;
 	bool	is_32bit;	/* Only valid if is_aarch32 is true */
@@ -79,7 +79,7 @@ static inline bool ignore_write(struct kvm_vcpu *vcpu,
 static inline bool read_zero(struct kvm_vcpu *vcpu,
 			     const struct sys_reg_params *p)
 {
-	*vcpu_reg(vcpu, p->Rt) = 0;
+	*p->val = 0;
 	return true;
 }
 
diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c b/arch/arm64/kvm/sys_regs_generic_v8.c
index 1e45768..c805576 100644
--- a/arch/arm64/kvm/sys_regs_generic_v8.c
+++ b/arch/arm64/kvm/sys_regs_generic_v8.c
@@ -37,7 +37,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
 	if (p->is_write)
 		return ignore_write(vcpu, p);
 
-	*vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, ACTLR_EL1);
+	*p->val = vcpu_sys_reg(vcpu, ACTLR_EL1);
 	return true;
 }
 
-- 
2.4.4

[PATCH 3/3] KVM: arm64: Get rid of old vcpu_reg()

[Pavel Fedin]

Using oldstyle vcpu_reg() accessor is proven to be inapproptiate and
unsafe on ARM64. This patch fixes the rest of use cases and completely
removes vcpu_reg() on ARM64.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/kvm/psci.c                  | 20 ++++++++++----------
 arch/arm64/include/asm/kvm_emulate.h | 11 +++--------
 arch/arm64/kvm/handle_exit.c         |  2 +-
 3 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
index 0b55696..a9b3b90 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -75,7 +75,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	unsigned long context_id;
 	phys_addr_t target_pc;
 
-	cpu_id = *vcpu_reg(source_vcpu, 1) & MPIDR_HWID_BITMASK;
+	cpu_id = vcpu_get_reg(source_vcpu, 1) & MPIDR_HWID_BITMASK;
 	if (vcpu_mode_is_32bit(source_vcpu))
 		cpu_id &= ~((u32) 0);
 
@@ -94,8 +94,8 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 			return PSCI_RET_INVALID_PARAMS;
 	}
 
-	target_pc = *vcpu_reg(source_vcpu, 2);
-	context_id = *vcpu_reg(source_vcpu, 3);
+	target_pc = vcpu_get_reg(source_vcpu, 2);
+	context_id = vcpu_get_reg(source_vcpu, 3);
 
 	kvm_reset_vcpu(vcpu);
 
@@ -114,7 +114,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
 	 * the general puspose registers are undefined upon CPU_ON.
 	 */
-	*vcpu_reg(vcpu, 0) = context_id;
+	vcpu_set_reg(vcpu, 0, context_id);
 	vcpu->arch.power_off = false;
 	smp_mb();		/* Make sure the above is visible */
 
@@ -134,8 +134,8 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 	struct kvm *kvm = vcpu->kvm;
 	struct kvm_vcpu *tmp;
 
-	target_affinity = *vcpu_reg(vcpu, 1);
-	lowest_affinity_level = *vcpu_reg(vcpu, 2);
+	target_affinity = vcpu_get_reg(vcpu, 1);
+	lowest_affinity_level = vcpu_get_reg(vcpu, 2);
 
 	/* Determine target affinity mask */
 	target_affinity_mask = psci_affinity_mask(lowest_affinity_level);
@@ -209,7 +209,7 @@ int kvm_psci_version(struct kvm_vcpu *vcpu)
 static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu)
 {
 	int ret = 1;
-	unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
+	unsigned long psci_fn = vcpu_get_reg(vcpu, 0) & ~((u32) 0);
 	unsigned long val;
 
 	switch (psci_fn) {
@@ -273,13 +273,13 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu)
 		break;
 	}
 
-	*vcpu_reg(vcpu, 0) = val;
+	vcpu_set_reg(vcpu, 0, val);
 	return ret;
 }
 
 static int kvm_psci_0_1_call(struct kvm_vcpu *vcpu)
 {
-	unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
+	unsigned long psci_fn = vcpu_get_reg(vcpu, 0) & ~((u32) 0);
 	unsigned long val;
 
 	switch (psci_fn) {
@@ -295,7 +295,7 @@ static int kvm_psci_0_1_call(struct kvm_vcpu *vcpu)
 		break;
 	}
 
-	*vcpu_reg(vcpu, 0) = val;
+	vcpu_set_reg(vcpu, 0, val);
 	return 1;
 }
 
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 5a182af..25a4021 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -100,15 +100,10 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
 }
 
 /*
- * vcpu_reg should always be passed a register number coming from a
- * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32
- * with banked registers.
+ * vcpu_get_reg and vcpu_set_reg should always be passed a register number
+ * coming from a read of ESR_EL2. Otherwise, it may give the wrong result on
+ * AArch32 with banked registers.
  */
-static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
-{
-	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num];
-}
-
 static inline unsigned long vcpu_get_reg(const struct kvm_vcpu *vcpu,
 					 u8 reg_num)
 {
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 68a0759..15f0477 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -37,7 +37,7 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
 	int ret;
 
-	trace_kvm_hvc_arm64(*vcpu_pc(vcpu), *vcpu_reg(vcpu, 0),
+	trace_kvm_hvc_arm64(*vcpu_pc(vcpu), vcpu_get_reg(vcpu, 0),
 			    kvm_vcpu_hvc_get_imm(vcpu));
 
 	ret = kvm_psci_call(vcpu);
-- 
2.4.4

Re: [PATCH 0/3] KVM: arm64: BUG FIX: Correctly handle zero register transfers

[Marc Zyngier]

On 03/12/15 09:58, Pavel Fedin wrote:
> ARM64 CPU has zero register which is read-only, with a value of 0.
> However, KVM currently incorrectly recognizes it being SP (because
> Rt == 31, and in struct user_pt_regs 'regs' array is followed by SP),
> resulting in invalid value being read, or even SP corruption on write.

No really. XZR and SP do share the same encoding.

> The problem has been discovered by performing an operation
> 
>  *((volatile int *)reg) = 0;
> 
> which compiles as "str xzr, [xx]", and resulted in strange values being
> written.

Interesting find. Which compiler is that?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Marc Zyngier]

Pavel,

On 03/12/15 09:58, Pavel Fedin wrote:
> System register accesses also use zero register for Rt == 31, and
> therefore using it will also result in getting SP value instead. This
> patch makes them also using new accessors, introduced by the previous
> patch.
> 
> Additionally, got rid of "massive hack" in kvm_handle_cp_64().

Looks good for a first drop - some comments below.

> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm64/kvm/sys_regs.c            | 79 ++++++++++++++++++++----------------
>  arch/arm64/kvm/sys_regs.h            |  4 +-
>  arch/arm64/kvm/sys_regs_generic_v8.c |  2 +-
>  3 files changed, 46 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87a64e8..a667228 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>  
>  	BUG_ON(!p->is_write);
>  
> -	val = *vcpu_reg(vcpu, p->Rt);
> +	val = *p->val;

Why does it have to be a pointer? You could just have "val = p->val" if
you carried the actual value instead of a pointer to the stack variable
holding that value.

>  	if (!p->is_aarch32) {
>  		vcpu_sys_reg(vcpu, r->reg) = val;
>  	} else {
> @@ -125,13 +125,10 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  			   const struct sys_reg_params *p,
>  			   const struct sys_reg_desc *r)
>  {
> -	u64 val;
> -
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p);
>  
> -	val = *vcpu_reg(vcpu, p->Rt);
> -	vgic_v3_dispatch_sgi(vcpu, val);
> +	vgic_v3_dispatch_sgi(vcpu, *p->val);
>  
>  	return true;
>  }
> @@ -153,7 +150,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>  	if (p->is_write) {
>  		return ignore_write(vcpu, p);
>  	} else {
> -		*vcpu_reg(vcpu, p->Rt) = (1 << 3);
> +		*p->val = (1 << 3);
>  		return true;
>  	}
>  }
> @@ -167,7 +164,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
>  	} else {
>  		u32 val;
>  		asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
> -		*vcpu_reg(vcpu, p->Rt) = val;
> +		*p->val = val;
>  		return true;
>  	}
>  }
> @@ -204,13 +201,13 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>  			    const struct sys_reg_desc *r)
>  {
>  	if (p->is_write) {
> -		vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> +		vcpu_sys_reg(vcpu, r->reg) = *p->val;
>  		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>  	} else {
> -		*vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg);
> +		*p->val = vcpu_sys_reg(vcpu, r->reg);
>  	}
>  
> -	trace_trap_reg(__func__, r->reg, p->is_write, *vcpu_reg(vcpu, p->Rt));
> +	trace_trap_reg(__func__, r->reg, p->is_write, *p->val);
>  
>  	return true;
>  }
> @@ -228,7 +225,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
>  			      const struct sys_reg_params *p,
>  			      u64 *dbg_reg)
>  {
> -	u64 val = *vcpu_reg(vcpu, p->Rt);
> +	u64 val = *p->val;
>  
>  	if (p->is_32bit) {
>  		val &= 0xffffffffUL;
> @@ -248,7 +245,7 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
>  	if (p->is_32bit)
>  		val &= 0xffffffffUL;
>  
> -	*vcpu_reg(vcpu, p->Rt) = val;
> +	*p->val = val;
>  }
>  
>  static inline bool trap_bvr(struct kvm_vcpu *vcpu,
> @@ -697,10 +694,10 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>  		u64 pfr = read_system_reg(SYS_ID_AA64PFR0_EL1);
>  		u32 el3 = !!cpuid_feature_extract_field(pfr, ID_AA64PFR0_EL3_SHIFT);
>  
> -		*vcpu_reg(vcpu, p->Rt) = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
> -					  (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
> -					  (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20) |
> -					  (6 << 16) | (el3 << 14) | (el3 << 12));
> +		*p->val = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
> +			   (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
> +			   (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20) |
> +			   (6 << 16) | (el3 << 14) | (el3 << 12));
>  		return true;
>  	}
>  }
> @@ -710,10 +707,10 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
>  			 const struct sys_reg_desc *r)
>  {
>  	if (p->is_write) {
> -		vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> +		vcpu_cp14(vcpu, r->reg) = *p->val;
>  		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>  	} else {
> -		*vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
> +		*p->val = vcpu_cp14(vcpu, r->reg);
>  	}
>  
>  	return true;
> @@ -740,12 +737,12 @@ static inline bool trap_xvr(struct kvm_vcpu *vcpu,
>  		u64 val = *dbg_reg;
>  
>  		val &= 0xffffffffUL;
> -		val |= *vcpu_reg(vcpu, p->Rt) << 32;
> +		val |= *p->val << 32;
>  		*dbg_reg = val;
>  
>  		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>  	} else {
> -		*vcpu_reg(vcpu, p->Rt) = *dbg_reg >> 32;
> +		*p->val = *dbg_reg >> 32;
>  	}
>  
>  	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> @@ -1062,12 +1059,14 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>  {
>  	struct sys_reg_params params;
>  	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +	int Rt = (hsr >> 5) & 0xf;
>  	int Rt2 = (hsr >> 10) & 0xf;
> +	unsigned long val;
>  
>  	params.is_aarch32 = true;
>  	params.is_32bit = false;
>  	params.CRm = (hsr >> 1) & 0xf;
> -	params.Rt = (hsr >> 5) & 0xf;
> +	params.val = &val;
>  	params.is_write = ((hsr & 1) == 0);
>  
>  	params.Op0 = 0;
> @@ -1076,15 +1075,12 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>  	params.CRn = 0;
>  
>  	/*
> -	 * Massive hack here. Store Rt2 in the top 32bits so we only
> -	 * have one register to deal with. As we use the same trap
> +	 * Make a 64-bit value out of Rt and Rt2. As we use the same trap
>  	 * backends between AArch32 and AArch64, we get away with it.
>  	 */
>  	if (params.is_write) {
> -		u64 val = *vcpu_reg(vcpu, params.Rt);
> -		val &= 0xffffffff;
> -		val |= *vcpu_reg(vcpu, Rt2) << 32;
> -		*vcpu_reg(vcpu, params.Rt) = val;
> +		val = vcpu_get_reg(vcpu, Rt) & 0xffffffff;
> +		val |= vcpu_get_reg(vcpu, Rt2) << 32;
>  	}
>  
>  	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
> @@ -1095,11 +1091,10 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>  	unhandled_cp_access(vcpu, &params);
>  
>  out:
> -	/* Do the opposite hack for the read side */
> +	/* Split up the value between registers for the read side */
>  	if (!params.is_write) {
> -		u64 val = *vcpu_reg(vcpu, params.Rt);
> -		val >>= 32;
> -		*vcpu_reg(vcpu, Rt2) = val;
> +		vcpu_set_reg(vcpu, Rt, val & 0xffffffff);
> +		vcpu_set_reg(vcpu, Rt, val >> 32);

Can you use higher_32bit/lower_32bit since you're touching that code?

>  	}
>  
>  	return 1;
> @@ -1118,21 +1113,27 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
>  {
>  	struct sys_reg_params params;
>  	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +	int Rt  = (hsr >> 5) & 0xf;
> +	unsigned long val = vcpu_get_reg(vcpu, Rt);
>  
>  	params.is_aarch32 = true;
>  	params.is_32bit = true;
>  	params.CRm = (hsr >> 1) & 0xf;
> -	params.Rt  = (hsr >> 5) & 0xf;
> +	params.val = &val;
>  	params.is_write = ((hsr & 1) == 0);
>  	params.CRn = (hsr >> 10) & 0xf;
>  	params.Op0 = 0;
>  	params.Op1 = (hsr >> 14) & 0x7;
>  	params.Op2 = (hsr >> 17) & 0x7;
>  
> -	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
> +	if (!emulate_cp(vcpu, &params, target_specific, nr_specific)) {
> +		vcpu_set_reg(vcpu, Rt, val);
>  		return 1;
> -	if (!emulate_cp(vcpu, &params, global, nr_global))
> +	}
> +	if (!emulate_cp(vcpu, &params, global, nr_global)) {
> +		vcpu_set_reg(vcpu, Rt, val);
>  		return 1;
> +	}

I'm not sure I'm 100% confident to do a writeback to the register if
that was a sysreg read.

Could you write it like this instead:
	if (!emulate_cp(vcpu, &params, target_specific, nr_specific) ||
	    !emulate_cp(vcpu, &params, global, nr_global)) {
		if (!params.is_write)
			vcpu_set_reg(vcpu, Rt, val);
		return 1;
	}
>  
>  	unhandled_cp_access(vcpu, &params);
>  	return 1;
> @@ -1230,6 +1231,9 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
>  	struct sys_reg_params params;
>  	unsigned long esr = kvm_vcpu_get_hsr(vcpu);
> +	int Rt = (esr >> 5) & 0x1f;
> +	unsigned long val = vcpu_get_reg(vcpu, Rt);
> +	int ret;
>  
>  	trace_kvm_handle_sys_reg(esr);
>  
> @@ -1240,10 +1244,13 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	params.CRn = (esr >> 10) & 0xf;
>  	params.CRm = (esr >> 1) & 0xf;
>  	params.Op2 = (esr >> 17) & 0x7;
> -	params.Rt = (esr >> 5) & 0x1f;
> +	params.val = &val;
>  	params.is_write = !(esr & 1);
>  
> -	return emulate_sys_reg(vcpu, &params);
> +	ret = emulate_sys_reg(vcpu, &params);
> +
> +	vcpu_set_reg(vcpu, Rt, val);

Same here. Clobbering the source register on a write feels unsafe.

> +	return ret;
>  }
>  
>  /******************************************************************************
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index eaa324e..3267518 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -28,7 +28,7 @@ struct sys_reg_params {
>  	u8	CRn;
>  	u8	CRm;
>  	u8	Op2;
> -	u8	Rt;
> +	u_long	*val;

This definitely should be a u64. I'd also prefer something like regval,
which is slightly more precise. And make it a direct variable, not a
pointer.

>  	bool	is_write;
>  	bool	is_aarch32;
>  	bool	is_32bit;	/* Only valid if is_aarch32 is true */
> @@ -79,7 +79,7 @@ static inline bool ignore_write(struct kvm_vcpu *vcpu,
>  static inline bool read_zero(struct kvm_vcpu *vcpu,
>  			     const struct sys_reg_params *p)
>  {
> -	*vcpu_reg(vcpu, p->Rt) = 0;
> +	*p->val = 0;
>  	return true;
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c b/arch/arm64/kvm/sys_regs_generic_v8.c
> index 1e45768..c805576 100644
> --- a/arch/arm64/kvm/sys_regs_generic_v8.c
> +++ b/arch/arm64/kvm/sys_regs_generic_v8.c
> @@ -37,7 +37,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
>  	if (p->is_write)
>  		return ignore_write(vcpu, p);
>  
> -	*vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, ACTLR_EL1);
> +	*p->val = vcpu_sys_reg(vcpu, ACTLR_EL1);
>  	return true;
>  }
>  
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/3] KVM: arm64: Correctly handle zero register during MMIO

[Marc Zyngier]

On 03/12/15 09:58, Pavel Fedin wrote:
> On ARM64 register index of 31 corresponds to both zero register and SP.
> However, all memory access instructions, use ZR as transfer register. SP
> is used only as a base register in indirect memory addressing, or by
> register-register arithmetics, which cannot be trapped here.
> 
> Correct emulation is achieved by introducing new register accessor
> functions, which can do special handling for reg_num == 31. These new
> accessors intentionally do not rely on old vcpu_reg() on ARM64, because
> it is to be removed. Since the affected code is shared by both ARM
> flavours, implementations of these accessors are also added to ARM32 code.
> 
> This patch fixes setting MMIO register to a random value (actually SP)
> instead of zero by something like:
> 
>  *((volatile int *)reg) = 0;
> 
> compilers tend to generate "str wzr, [xx]" here
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h   | 12 ++++++++++++
>  arch/arm/kvm/mmio.c                  |  5 +++--
>  arch/arm64/include/asm/kvm_emulate.h | 13 +++++++++++++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index a9c80a2..b7ff32e 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -28,6 +28,18 @@
>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>  
> +static inline unsigned long vcpu_get_reg(const struct kvm_vcpu *vcpu,
> +					 u8 reg_num)
> +{
> +	return *vcpu_reg(vcpu, reg_num);
> +}
> +
> +static inline void vcpu_set_reg(const struct kvm_vcpu *vcpu, u8 reg_num,
> +				unsigned long val)
> +{
> +	*vcpu_reg(vcpu, reg_num) = val;
> +}
> +
>  bool kvm_condition_valid(struct kvm_vcpu *vcpu);
>  void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr);
>  void kvm_inject_undefined(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 974b1c6..3a10c9f 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -115,7 +115,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
>  			       data);
>  		data = vcpu_data_host_to_guest(vcpu, data, len);
> -		*vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data;
> +		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>  	}
>  
>  	return 0;
> @@ -186,7 +186,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	rt = vcpu->arch.mmio_decode.rt;
>  
>  	if (is_write) {
> -		data = vcpu_data_guest_to_host(vcpu, *vcpu_reg(vcpu, rt), len);
> +		data = vcpu_data_guest_to_host(vcpu, vcpu_get_reg(vcpu, rt),
> +					       len);
>  
>  		trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, len, fault_ipa, data);
>  		mmio_write_buf(data_buf, len, data);
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 3ca894e..5a182af 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -109,6 +109,19 @@ static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
>  	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num];
>  }
>  
> +static inline unsigned long vcpu_get_reg(const struct kvm_vcpu *vcpu,
> +					 u8 reg_num)
> +{
> +	return (reg_num == 31) ? 0 : vcpu_gp_regs(vcpu)->regs.regs[reg_num];
> +}
> +
> +static inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num,
> +				unsigned long val)
> +{
> +	if (reg_num != 31)
> +		vcpu_gp_regs(vcpu)->regs.regs[reg_num] = val;
> +}
> +
>  /* Get vcpu SPSR for current mode */
>  static inline unsigned long *vcpu_spsr(const struct kvm_vcpu *vcpu)
>  {
> 

Thanks for finding this nasty one.

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH 0/3] KVM: arm64: BUG FIX: Correctly handle zero register transfers

[Pavel Fedin]

 Hello!

> > The problem has been discovered by performing an operation
> >
> >  *((volatile int *)reg) = 0;
> >
> > which compiles as "str xzr, [xx]", and resulted in strange values being
> > written.
> 
> Interesting find. Which compiler is that?

$ aarch64-linux-gnu-gcc --version
aarch64-linux-gnu-gcc (Linaro GCC 2014.11) 4.9.3 20141031 (prerelease)
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

 This is from my colleague who actually hit the bug by his driver. And i can reproduce the issue with different compiler version
using the following small testcase:
--- cut ---
p.fedin@fedinw7x64 /cygdrive/d/Projects/Test
$ cat test.c
volatile int *addr;

int test_val(int val)
{
    *addr = val;
}

int test_zero(void)
{
    *addr = 0;
}

p.fedin@fedinw7x64 /cygdrive/d/Projects/Test
$ aarch64-unknown-linux-gnu-gcc -O2 -c test.c

p.fedin@fedinw7x64 /cygdrive/d/Projects/Test
$ aarch64-unknown-linux-gnu-objdump -d test.o

test.o:     file format elf64-littleaarch64


Disassembly of section .text:

0000000000000000 <test_val>:
   0:   2a0003e2        mov     w2, w0
   4:   2a0103e0        mov     w0, w1
   8:   90000001        adrp    x1, 8 <test_val+0x8>
   c:   f9400021        ldr     x1, [x1]
  10:   b9000022        str     w2, [x1]
  14:   d65f03c0        ret

0000000000000018 <test_zero>:
  18:   90000001        adrp    x1, 8 <test_val+0x8>
  1c:   f9400021        ldr     x1, [x1]
  20:   b900003f        str     wzr, [x1]
  24:   d65f03c0        ret

p.fedin@fedinw7x64 /cygdrive/d/Projects/Test
$ aarch64-unknown-linux-gnu-gcc --version
aarch64-unknown-linux-gnu-gcc (GCC) 4.9.0
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
--- cut ---

 Isn't it legitimate to write from ZR to MMIO register?
 Another potential case is in our vgic-v3-switch.S:

	msr_s	ICH_HCR_EL2, xzr

 It's only because it is KVM code we have never discovered this problem yet. Somebody could write such a thing in some other place,
with some other register, which would be executed by KVM, and... boo...

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Pavel Fedin]

 Hello!

> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 87a64e8..a667228 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
> >
> >  	BUG_ON(!p->is_write);
> >
> > -	val = *vcpu_reg(vcpu, p->Rt);
> > +	val = *p->val;
> 
> Why does it have to be a pointer? You could just have "val = p->val" if
> you carried the actual value instead of a pointer to the stack variable
> holding that value.

 There's only one concern for pointer approach. Actually, this refactor is based on my vGICv3 live migration API patch set:
http://www.spinics.net/lists/kvm/msg124205.html
http://www.spinics.net/lists/kvm/msg124202.html

 It's simply more convenient to use a pointer for exchange with userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't
like to refactor the code again. What's your opinion on this?
 And of course i'll fix up the rest.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Marc Zyngier]

On 03/12/15 11:08, Pavel Fedin wrote:
> Hello!
> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c
>>> b/arch/arm64/kvm/sys_regs.c index 87a64e8..a667228 100644 ---
>>> a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@
>>> -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu
>>> *vcpu,
>>> 
>>> BUG_ON(!p->is_write);
>>> 
>>> -	val = *vcpu_reg(vcpu, p->Rt); +	val = *p->val;
>> 
>> Why does it have to be a pointer? You could just have "val =
>> p->val" if you carried the actual value instead of a pointer to the
>> stack variable holding that value.
> 
> There's only one concern for pointer approach. Actually, this
> refactor is based on my vGICv3 live migration API patch set: 
> http://www.spinics.net/lists/kvm/msg124205.html 
> http://www.spinics.net/lists/kvm/msg124202.html
> 
> It's simply more convenient to use a pointer for exchange with
> userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't like
> to refactor the code again. What's your opinion on this?

I still don't think this is a good idea. You can still store the value
as an integer in vgic_v3_cpu_regs_access(), and check the write property
to do the writeback on read. Which is the same thing I asked for in this
patch.

> And of course i'll fix up the rest.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] KVM: arm64: BUG FIX: Correctly handle zero register transfers

[Marc Zyngier]

On 03/12/15 10:53, Pavel Fedin wrote:
>  Hello!
> 
>>> The problem has been discovered by performing an operation
>>>
>>>  *((volatile int *)reg) = 0;
>>>
>>> which compiles as "str xzr, [xx]", and resulted in strange values being
>>> written.
>>
>> Interesting find. Which compiler is that?
> 
> $ aarch64-linux-gnu-gcc --version
> aarch64-linux-gnu-gcc (Linaro GCC 2014.11) 4.9.3 20141031 (prerelease)
> Copyright (C) 2014 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

OK. I was just wondering if that was a new thing or not.

[...]

>  Isn't it legitimate to write from ZR to MMIO register?
>  Another potential case is in our vgic-v3-switch.S:
> 
> 	msr_s	ICH_HCR_EL2, xzr
> 
>  It's only because it is KVM code we have never discovered this problem yet. Somebody could write such a thing in some other place,
> with some other register, which would be executed by KVM, and... boo...

I'm certainly not disputing that, this is a real bug that should be
fixed right now.

Looking forward to seeing your v2.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Pavel Fedin]

 Hello!

> > It's simply more convenient to use a pointer for exchange with
> > userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't like
> > to refactor the code again. What's your opinion on this?
> 
> I still don't think this is a good idea. You can still store the value
> as an integer in vgic_v3_cpu_regs_access(), and check the write property
> to do the writeback on read. Which is the same thing I asked for in this
> patch.

 Started doing this and found one more (big) reason against. All sysreg handlers have 'const struct sys_reg_params' declaration, and
callers, and their callers... This 'const' is all around the code, and it would take a separate huge patch to un-const'ify all this.
Does it worth that?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Marc Zyngier]

On 03/12/15 11:55, Pavel Fedin wrote:
>  Hello!
> 
>>> It's simply more convenient to use a pointer for exchange with
>>> userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't like
>>> to refactor the code again. What's your opinion on this?
>>
>> I still don't think this is a good idea. You can still store the value
>> as an integer in vgic_v3_cpu_regs_access(), and check the write property
>> to do the writeback on read. Which is the same thing I asked for in this
>> patch.
> 
>  Started doing this and found one more (big) reason against. All sysreg handlers have 'const struct sys_reg_params' declaration, and
> callers, and their callers... This 'const' is all around the code, and it would take a separate huge patch to un-const'ify all this.
> Does it worth that?

maz@approximate:~/Work/arm-platforms$ git diff --stat
 arch/arm64/kvm/sys_regs.c | 36 ++++++++++++++++++------------------
 arch/arm64/kvm/sys_regs.h |  2 +-
 2 files changed, 19 insertions(+), 19 deletions(-)

Hardly a big deal... You can have that as a separate patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...