KVM: x86: Check non-canonical addresses upon WRMSR

Upon WRMSR, the CPU should inject #GP if a non-canonical value (address) is written to certain MSRs. The behavior is "almost" identical for AMD and Intel (ignoring MSRs that are not implemented in either architecture since they would anyhow #GP). However, IA32_SYSENTER_ESP and IA32_SYSENTER_EIP cause #GP if non-canonical address is written on Intel but not on AMD (which ignores the top 32-bits). Accordingly, this patch injects a #GP on the MSRs which behave identically on Intel and AMD. To eliminate the differences between the architecutres, the value which is written to IA32_SYSENTER_ESP and IA32_SYSENTER_EIP is turned to canonical value before writing instead of injecting a #GP. Some references from Intel and AMD manuals: According to Intel SDM description of WRMSR instruction #GP is expected on WRMSR "If the source register contains a non-canonical address and ECX specifies one of the following MSRs: IA32_DS_AREA, IA32_FS_BASE, IA32_GS_BASE, IA32_KERNEL_GS_BASE, IA32_LSTAR, IA32_SYSENTER_EIP, IA32_SYSENTER_ESP." According to AMD manual instruction manual: LSTAR/CSTAR (SYSCALL): "The WRMSR instruction loads the target RIP into the LSTAR and CSTAR registers. If an RIP written by WRMSR is not in canonical form, a general-protection exception (#GP) occurs." IA32_GS_BASE and IA32_FS_BASE (WRFSBASE/WRGSBASE): "The address written to the base field must be in canonical form or a #GP fault will occur." IA32_KERNEL_GS_BASE (SWAPGS): "The address stored in the KernelGSbase MSR must be in canonical form." This patch fixes CVE-2014-3610. Cc: stable@vger.kernel.org Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86: Check non-canonical addresses upon WRMSR
Upon WRMSR, the CPU should inject #GP if a non-canonical value (address) is written to certain MSRs. The behavior is "almost" identical for AMD and Intel (ignoring MSRs that are not implemented in either architecture since they would anyhow #GP). However, IA32_SYSENTER_ESP and IA32_SYSENTER_EIP cause #GP if non-canonical address is written on Intel but not on AMD (which ignores the top 32-bits). Accordingly, this patch injects a #GP on the MSRs which behave identically on Intel and AMD. To eliminate the differences between the architecutres, the value which is written to IA32_SYSENTER_ESP and IA32_SYSENTER_EIP is turned to canonical value before writing instead of injecting a #GP. Some references from Intel and AMD manuals: According to Intel SDM description of WRMSR instruction #GP is expected on WRMSR "If the source register contains a non-canonical address and ECX specifies one of the following MSRs: IA32_DS_AREA, IA32_FS_BASE, IA32_GS_BASE, IA32_KERNEL_GS_BASE, IA32_LSTAR, IA32_SYSENTER_EIP, IA32_SYSENTER_ESP." According to AMD manual instruction manual: LSTAR/CSTAR (SYSCALL): "The WRMSR instruction loads the target RIP into the LSTAR and CSTAR registers. If an RIP written by WRMSR is not in canonical form, a general-protection exception (#GP) occurs." IA32_GS_BASE and IA32_FS_BASE (WRFSBASE/WRGSBASE): "The address written to the base field must be in canonical form or a #GP fault will occur." IA32_KERNEL_GS_BASE (SWAPGS): "The address stored in the KernelGSbase MSR must be in canonical form." This patch fixes CVE-2014-3610. Cc: stable@vger.kernel.org Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Nadav Amit · Paolo Bonzini · Eric Lee · Eric Lee · Eric Lee · Eric Lee
1 parent c3351dfabf
Showing 4 changed files with 42 additions and 3 deletions Side-by-side Diff
arch/x86/include/asm/kvm_host.h
arch/x86/kvm/svm.c
arch/x86/kvm/vmx.c
arch/x86/kvm/x86.c
@@ -989,6 +989,20 @@
 	kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
 }
  
+static inline u64 get_canonical(u64 la)
+{
+	return ((int64_t)la << 16) >> 16;
+}
+
+static inline bool is_noncanonical_address(u64 la)
+{
+#ifdef CONFIG_X86_64
+	return get_canonical(la) != la;
+#else
+	return false;
+#endif
+}
+
 #define TSS_IOPB_BASE_OFFSET 0x66
 #define TSS_BASE_SIZE 0x68
 #define TSS_IOPB_SIZE (65536 / 8)
@@ -3251,7 +3251,7 @@
 	msr.host_initiated = false;
  
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
-	if (svm_set_msr(&svm->vcpu, &msr)) {
+	if (kvm_set_msr(&svm->vcpu, &msr)) {
 		trace_kvm_msr_write_ex(ecx, data);
 		kvm_inject_gp(&svm->vcpu, 0);
 	} else {
@@ -5291,7 +5291,7 @@
 	msr.data = data;
 	msr.index = ecx;
 	msr.host_initiated = false;
-	if (vmx_set_msr(vcpu, &msr) != 0) {
+	if (kvm_set_msr(vcpu, &msr) != 0) {
 		trace_kvm_msr_write_ex(ecx, data);
 		kvm_inject_gp(vcpu, 0);
 		return 1;
@@ -987,7 +987,6 @@
 }
 EXPORT_SYMBOL_GPL(kvm_enable_efer_bits);
  
-
 /*
  * Writes msr value into into the appropriate "register".
  * Returns 0 on success, non-0 otherwise.
  
@@ -995,8 +994,34 @@
  */
 int kvm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 {
+	switch (msr->index) {
+	case MSR_FS_BASE:
+	case MSR_GS_BASE:
+	case MSR_KERNEL_GS_BASE:
+	case MSR_CSTAR:
+	case MSR_LSTAR:
+		if (is_noncanonical_address(msr->data))
+			return 1;
+		break;
+	case MSR_IA32_SYSENTER_EIP:
+	case MSR_IA32_SYSENTER_ESP:
+		/*
+		 * IA32_SYSENTER_ESP and IA32_SYSENTER_EIP cause #GP if
+		 * non-canonical address is written on Intel but not on
+		 * AMD (which ignores the top 32-bits, because it does
+		 * not implement 64-bit SYSENTER).
+		 *
+		 * 64-bit code should hence be able to write a non-canonical
+		 * value on AMD.  Making the address canonical ensures that
+		 * vmentry does not fail on Intel after writing a non-canonical
+		 * value, and that something deterministic happens if the guest
+		 * invokes 64-bit SYSENTER.
+		 */
+		msr->data = get_canonical(msr->data);
+	}
 	return kvm_x86_ops->set_msr(vcpu, msr);
 }
+EXPORT_SYMBOL_GPL(kvm_set_msr);
  
 /*
  * Adapt set_msr() to msr_io()'s calling convention
...	...	@@ -989,6 +989,20 @@
989	989	kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
990	990	}
991	991
	992	+static inline u64 get_canonical(u64 la)
	993	+{
	994	+ return ((int64_t)la << 16) >> 16;
	995	+}
	996	+
	997	+static inline bool is_noncanonical_address(u64 la)
	998	+{
	999	+#ifdef CONFIG_X86_64
	1000	+ return get_canonical(la) != la;
	1001	+#else
	1002	+ return false;
	1003	+#endif
	1004	+}
	1005	+
992	1006	#define TSS_IOPB_BASE_OFFSET 0x66
993	1007	#define TSS_BASE_SIZE 0x68
994	1008	#define TSS_IOPB_SIZE (65536 / 8)
...	...	@@ -3251,7 +3251,7 @@
3251	3251	msr.host_initiated = false;
3252	3252
3253	3253	svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
3254		- if (svm_set_msr(&svm->vcpu, &msr)) {
	3254	+ if (kvm_set_msr(&svm->vcpu, &msr)) {
3255	3255	trace_kvm_msr_write_ex(ecx, data);
3256	3256	kvm_inject_gp(&svm->vcpu, 0);
3257	3257	} else {
...	...	@@ -5291,7 +5291,7 @@
5291	5291	msr.data = data;
5292	5292	msr.index = ecx;
5293	5293	msr.host_initiated = false;
5294		- if (vmx_set_msr(vcpu, &msr) != 0) {
	5294	+ if (kvm_set_msr(vcpu, &msr) != 0) {
5295	5295	trace_kvm_msr_write_ex(ecx, data);
5296	5296	kvm_inject_gp(vcpu, 0);
5297	5297	return 1;
...	...	@@ -987,7 +987,6 @@
987	987	}
988	988	EXPORT_SYMBOL_GPL(kvm_enable_efer_bits);
989	989
990		-
991	990	/*
992	991	* Writes msr value into into the appropriate "register".
993	992	* Returns 0 on success, non-0 otherwise.
994	993
...	...	@@ -995,8 +994,34 @@
995	994	*/
996	995	int kvm_set_msr(struct kvm_vcpu vcpu, struct msr_data msr)
997	996	{
	997	+ switch (msr->index) {
	998	+ case MSR_FS_BASE:
	999	+ case MSR_GS_BASE:
	1000	+ case MSR_KERNEL_GS_BASE:
	1001	+ case MSR_CSTAR:
	1002	+ case MSR_LSTAR:
	1003	+ if (is_noncanonical_address(msr->data))
	1004	+ return 1;
	1005	+ break;
	1006	+ case MSR_IA32_SYSENTER_EIP:
	1007	+ case MSR_IA32_SYSENTER_ESP:
	1008	+ /*
	1009	+ * IA32_SYSENTER_ESP and IA32_SYSENTER_EIP cause #GP if
	1010	+ * non-canonical address is written on Intel but not on
	1011	+ * AMD (which ignores the top 32-bits, because it does
	1012	+ * not implement 64-bit SYSENTER).
	1013	+ *
	1014	+ * 64-bit code should hence be able to write a non-canonical
	1015	+ * value on AMD. Making the address canonical ensures that
	1016	+ * vmentry does not fail on Intel after writing a non-canonical
	1017	+ * value, and that something deterministic happens if the guest
	1018	+ * invokes 64-bit SYSENTER.
	1019	+ */
	1020	+ msr->data = get_canonical(msr->data);
	1021	+ }
998	1022	return kvm_x86_ops->set_msr(vcpu, msr);
999	1023	}
	1024	+EXPORT_SYMBOL_GPL(kvm_set_msr);
1000	1025
1001	1026	/*
1002	1027	* Adapt set_msr() to msr_io()'s calling convention