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

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

[Andrew Cooper]

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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

The way construct_vmcs() inherits vmx_secondary_exec_control is very
dangerous, and issues like this can creep in easily.  It is frankly a miracle
that there isn't an XSA here - the PML one almost was, and only isn't because
differnet piece of logic is broken.

I'd prefer to turn it into a feature whitelist approach, rather than
blacklist, but expressing the patch this way is the safest option for
backport, and I don't have time to rewrite the feature derivation logic at the
moment.
---
 xen/arch/x86/hvm/vmx/vmcs.c | 32 ++++++++++++++------------------
 xen/arch/x86/hvm/vmx/vvmx.c |  8 ++++++++
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index dec21d1..d6366c2 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1012,14 +1012,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) )
     {
@@ -1038,18 +1046,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 */
@@ -1133,9 +1132,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 a72b519..9f6ea5c 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1098,6 +1098,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));
@@ -1109,6 +1113,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);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 07.12.18 at 21:07, <andrew.cooper3@citrix.com> wrote:
> 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>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Andrew Cooper]

On 10/12/2018 16:01, Jan Beulich wrote:
>>>> On 07.12.18 at 21:07, <andrew.cooper3@citrix.com> wrote:
>> 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>

As this is blocking staging, I'm committing it right now.

If there are further concerns, I'll happily address them in a followup
patch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, December 8, 2018 4:07 AM
> 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>

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel