xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Validate host VMX MSRs before accessing them
@ 2016-06-09 10:14 Euan Harris
  2016-06-09 10:14 ` [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept() Euan Harris
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Euan Harris @ 2016-06-09 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: andrew.cooper3, kevin.tian, Euan Harris, jun.nakajima, jbeulich

nvmx_msr_read_intercept() does not check the prerequisites before
accessing MSR_IA32_VMX_PROCBASED_CTLS2, MSR_IA32_VMX_EPT_VPID_CAP,
MSR_IA32_VMX_VMFUNC on the host.   Accessing these MSRs from a nested
VMX guest running on a host which does not support them will cause
Xen to crash with a GPF.

Euan Harris (2):
  nested vmx: Fix comment typos in nvmx_msr_read_intercept()
  nested vmx: Validate host VMX MSRs before accessing them

 xen/arch/x86/hvm/vmx/vvmx.c |   32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept()
  2016-06-09 10:14 [PATCH 0/2] Validate host VMX MSRs before accessing them Euan Harris
@ 2016-06-09 10:14 ` Euan Harris
  2016-06-12  7:37   ` Tian, Kevin
  2016-06-09 10:14 ` [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them Euan Harris
  2016-06-13 12:13 ` [PATCH 0/2] " Andrew Cooper
  2 siblings, 1 reply; 8+ messages in thread
From: Euan Harris @ 2016-06-09 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: andrew.cooper3, kevin.tian, Euan Harris, jun.nakajima, jbeulich

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index faa8b69..d9493ff 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1852,7 +1852,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
     }
     case MSR_IA32_VMX_PINBASED_CTLS:
     case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-        /* 1-seetings */
+        /* 1-settings */
         data = PIN_BASED_EXT_INTR_MASK |
                PIN_BASED_NMI_EXITING |
                PIN_BASED_PREEMPT_TIMER;
@@ -1862,7 +1862,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
     case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
     {
         u32 default1_bits = VMX_PROCBASED_CTLS_DEFAULT1;
-        /* 1-seetings */
+        /* 1-settings */
         data = CPU_BASED_HLT_EXITING |
                CPU_BASED_VIRTUAL_INTR_PENDING |
                CPU_BASED_CR8_LOAD_EXITING |
@@ -1894,7 +1894,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         break;
     }
     case MSR_IA32_VMX_PROCBASED_CTLS2:
-        /* 1-seetings */
+        /* 1-settings */
         data = SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING |
                SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
                SECONDARY_EXEC_ENABLE_VPID |
@@ -1906,7 +1906,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         break;
     case MSR_IA32_VMX_EXIT_CTLS:
     case MSR_IA32_VMX_TRUE_EXIT_CTLS:
-        /* 1-seetings */
+        /* 1-settings */
         data = VM_EXIT_ACK_INTR_ON_EXIT |
                VM_EXIT_IA32E_MODE |
                VM_EXIT_SAVE_PREEMPT_TIMER |
@@ -1919,7 +1919,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         break;
     case MSR_IA32_VMX_ENTRY_CTLS:
     case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
-        /* 1-seetings */
+        /* 1-settings */
         data = VM_ENTRY_LOAD_GUEST_PAT |
                VM_ENTRY_LOAD_GUEST_EFER |
                VM_ENTRY_LOAD_PERF_GLOBAL_CTRL |
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them
  2016-06-09 10:14 [PATCH 0/2] Validate host VMX MSRs before accessing them Euan Harris
  2016-06-09 10:14 ` [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept() Euan Harris
@ 2016-06-09 10:14 ` Euan Harris
  2016-06-09 12:47   ` Jan Beulich
  2016-06-13 12:13 ` [PATCH 0/2] " Andrew Cooper
  2 siblings, 1 reply; 8+ messages in thread
From: Euan Harris @ 2016-06-09 10:14 UTC (permalink / raw)
  To: xen-devel; +Cc: andrew.cooper3, kevin.tian, Euan Harris, jun.nakajima, jbeulich

Some VMX MSRs may not exist on certain processor models, or may
be disabled because of configuration settings.   It is only safe to
access these MSRs if configuration flags in other MSRs are set.  These
prerequisites are listed in the Intel 64 and IA-32 Architectures
Software Developer’s Manual, Vol 3, Appendix A.

nvmx_msr_read_intercept() does not check the prerequisites before
accessing MSR_IA32_VMX_PROCBASED_CTLS2, MSR_IA32_VMX_EPT_VPID_CAP,
MSR_IA32_VMX_VMFUNC on the host.   Accessing these MSRs from a nested
VMX guest running on a host which does not support them will cause
Xen to crash with a GPF.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d9493ff..ddc25bf 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1820,11 +1820,20 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         return 0;
 
     /*
-     * Those MSRs are available only when bit 55 of
-     * MSR_IA32_VMX_BASIC is set.
+     * These MSRs are only available when flags in other MSRs are set.
+     * These prerequisites are listed in the Intel 64 and IA-32
+     * Architectures Software Developer’s Manual, Vol 3, Appendix A.
      */
-    switch ( msr )
-    {
+    switch ( msr ) { case MSR_IA32_VMX_PROCBASED_CTLS2:
+        if ( !cpu_has_vmx_secondary_exec_control )
+            return 0;
+        break;
+
+    case MSR_IA32_VMX_EPT_VPID_CAP:
+        if ( !(cpu_has_vmx_ept || cpu_has_vmx_vpid) )
+            return 0;
+        break;
+
     case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
     case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
     case MSR_IA32_VMX_TRUE_EXIT_CTLS:
@@ -1832,6 +1841,11 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
             return 0;
         break;
+
+    case MSR_IA32_VMX_VMFUNC:
+        if ( !cpu_has_vmx_vmfunc )
+            return 0;
+        break;
     }
 
     rdmsrl(msr, host_data);
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them
  2016-06-09 10:14 ` [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them Euan Harris
@ 2016-06-09 12:47   ` Jan Beulich
  2016-06-09 13:20     ` Euan Harris
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-06-09 12:47 UTC (permalink / raw)
  To: Euan Harris; +Cc: andrew.cooper3, kevin.tian, jun.nakajima, xen-devel

>>> On 09.06.16 at 12:14, <euan.harris@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1820,11 +1820,20 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
>          return 0;
>  
>      /*
> -     * Those MSRs are available only when bit 55 of
> -     * MSR_IA32_VMX_BASIC is set.
> +     * These MSRs are only available when flags in other MSRs are set.
> +     * These prerequisites are listed in the Intel 64 and IA-32
> +     * Architectures Software Developer’s Manual, Vol 3, Appendix A.
>       */
> -    switch ( msr )
> -    {
> +    switch ( msr ) { case MSR_IA32_VMX_PROCBASED_CTLS2:

I hope you didn't really mean to produce a garbled line like this?
With proper formatting restored
Reviewed-by: Jan Beulich <jbeulich@suse.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them
  2016-06-09 12:47   ` Jan Beulich
@ 2016-06-09 13:20     ` Euan Harris
  2016-06-12  7:39       ` Tian, Kevin
  0 siblings, 1 reply; 8+ messages in thread
From: Euan Harris @ 2016-06-09 13:20 UTC (permalink / raw)
  To: Jan Beulich; +Cc: andrew.cooper3, kevin.tian, jun.nakajima, xen-devel

On Thu, Jun 09, 2016 at 06:47:55AM -0600, Jan Beulich wrote:
> >      /*
> > -     * Those MSRs are available only when bit 55 of
> > -     * MSR_IA32_VMX_BASIC is set.
> > +     * These MSRs are only available when flags in other MSRs are set.
> > +     * These prerequisites are listed in the Intel 64 and IA-32
> > +     * Architectures Software Developer’s Manual, Vol 3, Appendix A.
> >       */
> > -    switch ( msr )
> > -    {
> > +    switch ( msr ) { case MSR_IA32_VMX_PROCBASED_CTLS2:
> 
> I hope you didn't really mean to produce a garbled line like this?
> With proper formatting restored
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Oops, definitely not.   I think this happened when I was re-wrapping
the comment above - !}fmt went further than I intended.

Sorry,
Euan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept()
  2016-06-09 10:14 ` [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept() Euan Harris
@ 2016-06-12  7:37   ` Tian, Kevin
  0 siblings, 0 replies; 8+ messages in thread
From: Tian, Kevin @ 2016-06-12  7:37 UTC (permalink / raw)
  To: Euan Harris, xen-devel; +Cc: andrew.cooper3, Nakajima, Jun, jbeulich

> From: Euan Harris [mailto:euan.harris@citrix.com]
> Sent: Thursday, June 09, 2016 6:14 PM
> 
> Signed-off-by: Euan Harris <euan.harris@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them
  2016-06-09 13:20     ` Euan Harris
@ 2016-06-12  7:39       ` Tian, Kevin
  0 siblings, 0 replies; 8+ messages in thread
From: Tian, Kevin @ 2016-06-12  7:39 UTC (permalink / raw)
  To: Euan Harris, Jan Beulich; +Cc: andrew.cooper3, Nakajima, Jun, xen-devel

> From: Euan Harris [mailto:euan.harris@citrix.com]
> Sent: Thursday, June 09, 2016 9:21 PM
> 
> On Thu, Jun 09, 2016 at 06:47:55AM -0600, Jan Beulich wrote:
> > >      /*
> > > -     * Those MSRs are available only when bit 55 of
> > > -     * MSR_IA32_VMX_BASIC is set.
> > > +     * These MSRs are only available when flags in other MSRs are set.
> > > +     * These prerequisites are listed in the Intel 64 and IA-32
> > > +     * Architectures Software Developer’s Manual, Vol 3, Appendix A.
> > >       */
> > > -    switch ( msr )
> > > -    {
> > > +    switch ( msr ) { case MSR_IA32_VMX_PROCBASED_CTLS2:
> >
> > I hope you didn't really mean to produce a garbled line like this?
> > With proper formatting restored
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Oops, definitely not.   I think this happened when I was re-wrapping
> the comment above - !}fmt went further than I intended.
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>, with above fixed.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] Validate host VMX MSRs before accessing them
  2016-06-09 10:14 [PATCH 0/2] Validate host VMX MSRs before accessing them Euan Harris
  2016-06-09 10:14 ` [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept() Euan Harris
  2016-06-09 10:14 ` [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them Euan Harris
@ 2016-06-13 12:13 ` Andrew Cooper
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2016-06-13 12:13 UTC (permalink / raw)
  To: Euan Harris, xen-devel; +Cc: kevin.tian, jun.nakajima, jbeulich

On 09/06/16 11:14, Euan Harris wrote:
> nvmx_msr_read_intercept() does not check the prerequisites before
> accessing MSR_IA32_VMX_PROCBASED_CTLS2, MSR_IA32_VMX_EPT_VPID_CAP,
> MSR_IA32_VMX_VMFUNC on the host.   Accessing these MSRs from a nested
> VMX guest running on a host which does not support them will cause
> Xen to crash with a GPF.
>
> Euan Harris (2):
>   nested vmx: Fix comment typos in nvmx_msr_read_intercept()
>   nested vmx: Validate host VMX MSRs before accessing them
>
>  xen/arch/x86/hvm/vmx/vvmx.c |   32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>

Committed, with the indicated fixup.

Thanks,

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-13 12:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-09 10:14 [PATCH 0/2] Validate host VMX MSRs before accessing them Euan Harris
2016-06-09 10:14 ` [PATCH 1/2] nested vmx: Fix comment typos in nvmx_msr_read_intercept() Euan Harris
2016-06-12  7:37   ` Tian, Kevin
2016-06-09 10:14 ` [PATCH 2/2] nested vmx: Validate host VMX MSRs before accessing them Euan Harris
2016-06-09 12:47   ` Jan Beulich
2016-06-09 13:20     ` Euan Harris
2016-06-12  7:39       ` Tian, Kevin
2016-06-13 12:13 ` [PATCH 0/2] " Andrew Cooper

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).