From 83a7abe67701bd1af9d19bbd762faba05c686297 Mon Sep 17 00:00:00 2001 From: Michael Young Date: Oct 24 2018 22:16:51 +0000 Subject: x86: Nested VT-x usable even when disabled [XSA-278] --- diff --git a/xen.spec b/xen.spec index d91ef31..ee6e508 100644 --- a/xen.spec +++ b/xen.spec @@ -60,7 +60,7 @@ Summary: Xen is a virtual machine monitor Name: xen Version: 4.11.0 -Release: 7%{?dist} +Release: 8%{?dist} Group: Development/Libraries License: GPLv2+ and LGPLv2+ and BSD URL: http://xen.org/ @@ -120,6 +120,7 @@ Patch42: xen.stubdom.build.patch Patch43: xsa273.patch Patch44: xen.vwprintw.fix.patch Patch45: xen.python.env.patch +Patch46: xsa278-4.11.patch %if %build_qemutrad @@ -331,6 +332,7 @@ manage Xen virtual machines. %patch43 -p1 %patch44 -p1 %patch45 -p1 +%patch46 -p1 # qemu-xen-traditional patches pushd tools/qemu-xen-traditional @@ -872,6 +874,9 @@ fi %endif %changelog +* Wed Oct 24 2018 Michael Young - 4.11.0-8 +- x86: Nested VT-x usable even when disabled [XSA-278] + * Tue Sep 18 2018 Michael Young - 4.11.0-7 - set /usr/bin/python2 at the start of another python script diff --git a/xsa278-4.11.patch b/xsa278-4.11.patch new file mode 100644 index 0000000..9e14849 --- /dev/null +++ b/xsa278-4.11.patch @@ -0,0 +1,326 @@ +From: Andrew Cooper +Subject: x86/vvmx: Disallow the use of VT-x instructions when nested virt is disabled + +c/s ac6a4500b "vvmx: set vmxon_region_pa of vcpu out of VMX operation to an +invalid address" was a real bugfix as described, but has a very subtle bug +which results in all VT-x instructions being usable by a guest. + +The toolstack constructs a guest by issuing: + + XEN_DOMCTL_createdomain + XEN_DOMCTL_max_vcpus + +and optionally later, HVMOP_set_param to enable nested virt. + +As a result, the call to nvmx_vcpu_initialise() in hvm_vcpu_initialise() +(which is what makes the above patch look correct during review) is actually +dead code. In practice, nvmx_vcpu_initialise() first gets called when nested +virt is enabled, which is typically never. + +As a result, the zeroed memory of struct vcpu causes nvmx_vcpu_in_vmx() to +return true before nested virt is enabled for the guest. + +Fixing the order of initialisation is a work in progress for other reasons, +but not viable for security backports. + +A compounding factor is that the vmexit handlers for all instructions, other +than VMXON, pass 0 into vmx_inst_check_privilege()'s vmxop_check parameter, +which skips the CR4.VMXE check. (This is one of many reasons why nested virt +isn't a supported feature yet.) + +However, the overall result is that when nested virt is not enabled by the +toolstack (i.e. the default configuration for all production guests), the VT-x +instructions (other than VMXON) are actually usable, and Xen very quickly +falls over the fact that the nvmx structure is uninitialised. + +In order to fail safe in the supported case, re-implement all the VT-x +instruction handling using a single function with a common prologue, covering +all the checks which should cause #UD or #GP faults. This deliberately +doesn't use any state from the nvmx structure, in case there are other lurking +issues. + +This is XSA-278 + +Reported-by: Sergey Dyasli +Signed-off-by: Andrew Cooper +Reviewed-by: Sergey Dyasli + +diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c +index a6415f0..a4d2829 100644 +--- a/xen/arch/x86/hvm/vmx/vmx.c ++++ b/xen/arch/x86/hvm/vmx/vmx.c +@@ -3982,57 +3982,17 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) + break; + + case EXIT_REASON_VMXOFF: +- if ( nvmx_handle_vmxoff(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMXON: +- if ( nvmx_handle_vmxon(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMCLEAR: +- if ( nvmx_handle_vmclear(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMPTRLD: +- if ( nvmx_handle_vmptrld(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMPTRST: +- if ( nvmx_handle_vmptrst(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMREAD: +- if ( nvmx_handle_vmread(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMWRITE: +- if ( nvmx_handle_vmwrite(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMLAUNCH: +- if ( nvmx_handle_vmlaunch(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_VMRESUME: +- if ( nvmx_handle_vmresume(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_INVEPT: +- if ( nvmx_handle_invept(regs) == X86EMUL_OKAY ) +- update_guest_eip(); +- break; +- + case EXIT_REASON_INVVPID: +- if ( nvmx_handle_invvpid(regs) == X86EMUL_OKAY ) ++ if ( nvmx_handle_vmx_insn(regs, exit_reason) == X86EMUL_OKAY ) + update_guest_eip(); + break; + +diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c +index e97db33..88cb58c 100644 +--- a/xen/arch/x86/hvm/vmx/vvmx.c ++++ b/xen/arch/x86/hvm/vmx/vvmx.c +@@ -1470,7 +1470,7 @@ void nvmx_switch_guest(void) + * VMX instructions handling + */ + +-int nvmx_handle_vmxon(struct cpu_user_regs *regs) ++static int nvmx_handle_vmxon(struct cpu_user_regs *regs) + { + struct vcpu *v=current; + struct nestedvmx *nvmx = &vcpu_2_nvmx(v); +@@ -1522,7 +1522,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_vmxoff(struct cpu_user_regs *regs) ++static int nvmx_handle_vmxoff(struct cpu_user_regs *regs) + { + struct vcpu *v=current; + struct nestedvmx *nvmx = &vcpu_2_nvmx(v); +@@ -1611,7 +1611,7 @@ static int nvmx_vmresume(struct vcpu *v, struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_vmresume(struct cpu_user_regs *regs) ++static int nvmx_handle_vmresume(struct cpu_user_regs *regs) + { + bool_t launched; + struct vcpu *v = current; +@@ -1645,7 +1645,7 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs) + return nvmx_vmresume(v,regs); + } + +-int nvmx_handle_vmlaunch(struct cpu_user_regs *regs) ++static int nvmx_handle_vmlaunch(struct cpu_user_regs *regs) + { + bool_t launched; + struct vcpu *v = current; +@@ -1688,7 +1688,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs) + return rc; + } + +-int nvmx_handle_vmptrld(struct cpu_user_regs *regs) ++static int nvmx_handle_vmptrld(struct cpu_user_regs *regs) + { + struct vcpu *v = current; + struct vmx_inst_decoded decode; +@@ -1759,7 +1759,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_vmptrst(struct cpu_user_regs *regs) ++static int nvmx_handle_vmptrst(struct cpu_user_regs *regs) + { + struct vcpu *v = current; + struct vmx_inst_decoded decode; +@@ -1784,7 +1784,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_vmclear(struct cpu_user_regs *regs) ++static int nvmx_handle_vmclear(struct cpu_user_regs *regs) + { + struct vcpu *v = current; + struct vmx_inst_decoded decode; +@@ -1836,7 +1836,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_vmread(struct cpu_user_regs *regs) ++static int nvmx_handle_vmread(struct cpu_user_regs *regs) + { + struct vcpu *v = current; + struct vmx_inst_decoded decode; +@@ -1878,7 +1878,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_vmwrite(struct cpu_user_regs *regs) ++static int nvmx_handle_vmwrite(struct cpu_user_regs *regs) + { + struct vcpu *v = current; + struct vmx_inst_decoded decode; +@@ -1926,7 +1926,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_invept(struct cpu_user_regs *regs) ++static int nvmx_handle_invept(struct cpu_user_regs *regs) + { + struct vmx_inst_decoded decode; + unsigned long eptp; +@@ -1954,7 +1954,7 @@ int nvmx_handle_invept(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + +-int nvmx_handle_invvpid(struct cpu_user_regs *regs) ++static int nvmx_handle_invvpid(struct cpu_user_regs *regs) + { + struct vmx_inst_decoded decode; + unsigned long vpid; +@@ -1980,6 +1980,81 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs) + return X86EMUL_OKAY; + } + ++int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason) ++{ ++ struct vcpu *curr = current; ++ int ret; ++ ++ if ( !(curr->arch.hvm_vcpu.guest_cr[4] & X86_CR4_VMXE) || ++ !nestedhvm_enabled(curr->domain) || ++ (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? 8 : 2)) ) ++ { ++ hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); ++ return X86EMUL_EXCEPTION; ++ } ++ ++ if ( vmx_get_cpl() > 0 ) ++ { ++ hvm_inject_hw_exception(TRAP_gp_fault, 0); ++ return X86EMUL_EXCEPTION; ++ } ++ ++ switch ( exit_reason ) ++ { ++ case EXIT_REASON_VMXOFF: ++ ret = nvmx_handle_vmxoff(regs); ++ break; ++ ++ case EXIT_REASON_VMXON: ++ ret = nvmx_handle_vmxon(regs); ++ break; ++ ++ case EXIT_REASON_VMCLEAR: ++ ret = nvmx_handle_vmclear(regs); ++ break; ++ ++ case EXIT_REASON_VMPTRLD: ++ ret = nvmx_handle_vmptrld(regs); ++ break; ++ ++ case EXIT_REASON_VMPTRST: ++ ret = nvmx_handle_vmptrst(regs); ++ break; ++ ++ case EXIT_REASON_VMREAD: ++ ret = nvmx_handle_vmread(regs); ++ break; ++ ++ case EXIT_REASON_VMWRITE: ++ ret = nvmx_handle_vmwrite(regs); ++ break; ++ ++ case EXIT_REASON_VMLAUNCH: ++ ret = nvmx_handle_vmlaunch(regs); ++ break; ++ ++ case EXIT_REASON_VMRESUME: ++ ret = nvmx_handle_vmresume(regs); ++ break; ++ ++ case EXIT_REASON_INVEPT: ++ ret = nvmx_handle_invept(regs); ++ break; ++ ++ case EXIT_REASON_INVVPID: ++ ret = nvmx_handle_invvpid(regs); ++ break; ++ ++ default: ++ ASSERT_UNREACHABLE(); ++ domain_crash(curr->domain); ++ ret = X86EMUL_UNHANDLEABLE; ++ break; ++ } ++ ++ return ret; ++} ++ + #define __emul_value(enable1, default1) \ + ((enable1 | default1) << 32 | (default1)) + +diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h +index 9ea35eb..fc4a8d1 100644 +--- a/xen/include/asm-x86/hvm/vmx/vvmx.h ++++ b/xen/include/asm-x86/hvm/vmx/vvmx.h +@@ -94,9 +94,6 @@ void nvmx_domain_relinquish_resources(struct domain *d); + + bool_t nvmx_ept_enabled(struct vcpu *v); + +-int nvmx_handle_vmxon(struct cpu_user_regs *regs); +-int nvmx_handle_vmxoff(struct cpu_user_regs *regs); +- + #define EPT_TRANSLATE_SUCCEED 0 + #define EPT_TRANSLATE_VIOLATION 1 + #define EPT_TRANSLATE_MISCONFIG 2 +@@ -191,15 +188,7 @@ enum vmx_insn_errno set_vvmcs_real_safe(const struct vcpu *, u32 encoding, + uint64_t get_shadow_eptp(struct vcpu *v); + + void nvmx_destroy_vmcs(struct vcpu *v); +-int nvmx_handle_vmptrld(struct cpu_user_regs *regs); +-int nvmx_handle_vmptrst(struct cpu_user_regs *regs); +-int nvmx_handle_vmclear(struct cpu_user_regs *regs); +-int nvmx_handle_vmread(struct cpu_user_regs *regs); +-int nvmx_handle_vmwrite(struct cpu_user_regs *regs); +-int nvmx_handle_vmresume(struct cpu_user_regs *regs); +-int nvmx_handle_vmlaunch(struct cpu_user_regs *regs); +-int nvmx_handle_invept(struct cpu_user_regs *regs); +-int nvmx_handle_invvpid(struct cpu_user_regs *regs); ++int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason); + int nvmx_msr_read_intercept(unsigned int msr, + u64 *msr_content); +