x86/VT-x: Don't activate VMCS Shadowing outside of nested vmx mode

By default on capable hardware, SECONDARY_EXEC_ENABLE_VMCS_SHADOWING is
activated unilaterally.  The VMCS Link pointer is initialised to ~0, but the
VMREAD/VMWRITE bitmap pointers are not.

This causes the 16bit IVT and Bios Data Area get interpreted as the read/write
permission bitmap for guests which blindly execute VMREAD/VMWRITE
instructions.

This is not a security issue because the VMCS Link pointer being ~0 causes
VMREAD/VMWRITE to complete with VMFailInvalid (rather than modifying a
potential shadow VMCS), and the contents of MFN 0 has already been determined
not to contain any interesting data because of L1TF's ability to read that 4k
frame.

Leave VMCS Shadowing disabled by default, and toggle it in
nvmx_{set,clear}_vmcs_pointer().  This isn't the most efficient course of
action, but it is the most simple way of leaving nested-virt working as it did
before.

While editing construct_vmcs(), collect all default secondary_exec_control
modifications together.  The disabling of PML is latently buggy because it
happens after secondary_exec_control are written into the VMCS, although there
is an unconditional update later which writes the correct value into hardware.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
master commit: 75ce36eb72cb93e8a3c9f60fd5e697067921d712
master date: 2018-12-10 16:24:08 +0000

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 49b86473b12e48e5e0a9e4b3eb2999c0565d36c3..345bfbf6fca457fffc3334f07bd8cd00a0a0f72e 100644 (file)
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1032,14 +1032,22 @@ static int construct_vmcs(struct vcpu *v)
     v->arch.hvm_vmx.secondary_exec_control = vmx_secondary_exec_control;
 
     /*
-     * Disable descriptor table exiting: It's controlled by the VM event
-     * monitor requesting it.
+     * Disable features which we don't want active by default:
+     *  - Descriptor table exiting only if wanted by introspection
+     *  - x2APIC - default is xAPIC mode
+     *  - VPID settings chosen at VMEntry time
+     *  - VMCS Shadowing only when in nested VMX mode
+     *  - PML only when logdirty is active
+     *  - VMFUNC/#VE only if wanted by altp2m
      */
     v->arch.hvm_vmx.secondary_exec_control &=
-        ~SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING;
-
-    /* Disable VPID for now: we decide when to enable it on VMENTER. */
-    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
+        ~(SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING |
+          SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
+          SECONDARY_EXEC_ENABLE_VPID |
+          SECONDARY_EXEC_ENABLE_VMCS_SHADOWING |
+          SECONDARY_EXEC_ENABLE_PML |
+          SECONDARY_EXEC_ENABLE_VM_FUNCTIONS |
+          SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
 
     if ( paging_mode_hap(d) )
     {
@@ -1058,18 +1066,9 @@ static int construct_vmcs(struct vcpu *v)
         vmentry_ctl &= ~VM_ENTRY_LOAD_GUEST_PAT;
     }
 
-    /* Disable Virtualize x2APIC mode by default. */
-    v->arch.hvm_vmx.secondary_exec_control &=
-        ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
-
     /* Do not enable Monitor Trap Flag unless start single step debug */
     v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG;
 
-    /* Disable VMFUNC and #VE for now: they may be enabled later by altp2m. */
-    v->arch.hvm_vmx.secondary_exec_control &=
-        ~(SECONDARY_EXEC_ENABLE_VM_FUNCTIONS |
-          SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
-
     if ( !has_vlapic(d) )
     {
         /* Disable virtual apics, TPR */
@@ -1153,9 +1152,6 @@ static int construct_vmcs(struct vcpu *v)
         __vmwrite(POSTED_INTR_NOTIFICATION_VECTOR, posted_intr_vector);
     }
 
-    /* Disable PML anyway here as it will only be enabled in log dirty mode */
-    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
-
     /* Host data selectors. */
     __vmwrite(HOST_SS_SELECTOR, __HYPERVISOR_DS);
     __vmwrite(HOST_DS_SELECTOR, __HYPERVISOR_DS);

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 20e7d4bf107f9fea4b7f779f8878cbd82de925a8..a3cce670d118d107bbb8bf4a74467e25d336858a 100644 (file)
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1124,6 +1124,10 @@ static void nvmx_set_vmcs_pointer(struct vcpu *v, struct vmcs_struct *vvmcs)
 
     __vmpclear(vvmcs_maddr);
     vvmcs->vmcs_revision_id |= VMCS_RID_TYPE_MASK;
+    v->arch.hvm_vmx.secondary_exec_control |=
+        SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+              v->arch.hvm_vmx.secondary_exec_control);
     __vmwrite(VMCS_LINK_POINTER, vvmcs_maddr);
     __vmwrite(VMREAD_BITMAP, page_to_maddr(v->arch.hvm_vmx.vmread_bitmap));
     __vmwrite(VMWRITE_BITMAP, page_to_maddr(v->arch.hvm_vmx.vmwrite_bitmap));
@@ -1135,6 +1139,10 @@ static void nvmx_clear_vmcs_pointer(struct vcpu *v, struct vmcs_struct *vvmcs)
 
     __vmpclear(vvmcs_maddr);
     vvmcs->vmcs_revision_id &= ~VMCS_RID_TYPE_MASK;
+    v->arch.hvm_vmx.secondary_exec_control &=
+        ~SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+              v->arch.hvm_vmx.secondary_exec_control);
     __vmwrite(VMCS_LINK_POINTER, ~0ul);
     __vmwrite(VMREAD_BITMAP, 0);
     __vmwrite(VMWRITE_BITMAP, 0);