[PATCH 0/3] vvmx: fix L1 vmxon

[PATCH 0/3] vvmx: fix L1 vmxon

[Haozhong Zhang]

This patchset fixes bugs and adds missing checks in nvmx_handle_vmxon(),
in order to make it more consistent to Intel SDM (section "VMXON - Enter
VMX Operation" in Vol 3).

Haozhong Zhang (3):
  vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address
  vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation
  vvmx: check the operand of L1 vmxon

 xen/arch/x86/hvm/vmx/vvmx.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

-- 
2.10.1


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

[PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Haozhong Zhang]

nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
also a valid VMXON region address. If L1 hypervisor had set the VMXON
region address to 0, the check in nvmx_handle_vmxon() will be skipped.
Fix this problem by using an invalid VMXON region address for vcpu
out of VMX operation.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e6e9ebd..f5637eb 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
 
 static void nvmx_purge_vvmcs(struct vcpu *v);
 
+/*
+ * When a vcpu is out of VMXON region, set its vmxon_region_pa to
+ * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
+ * VMXON region address.
+ */
+#define INVALID_VMXON_REGION_PA (~0UL)
+
+static bool nvmx_vcpu_in_vmx(struct vcpu *v)
+{
+    return vcpu_2_nvmx(v).vmxon_region_pa != INVALID_VMXON_REGION_PA;
+}
+
 #define VMCS_BUF_SIZE 100
 
 int nvmx_cpu_up_prepare(unsigned int cpu)
@@ -107,7 +119,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
 
     nvmx->ept.enabled = 0;
     nvmx->guest_vpid = 0;
-    nvmx->vmxon_region_pa = 0;
+    nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
     nvcpu->nv_vvmcx = NULL;
     nvcpu->nv_vvmcxaddr = VMCX_EADDR;
     nvmx->intr.intr_info = 0;
@@ -357,7 +369,7 @@ static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
              !(v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_VMXE) )
             goto invalid_op;
     }
-    else if ( !vcpu_2_nvmx(v).vmxon_region_pa )
+    else if ( !nvmx_vcpu_in_vmx(v) )
         goto invalid_op;
 
     hvm_get_segment_register(v, x86_seg_cs, &cs);
@@ -1384,7 +1396,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    if ( nvmx->vmxon_region_pa )
+    if ( nvmx_vcpu_in_vmx(v) )
         gdprintk(XENLOG_WARNING, 
                  "vmxon again: orig %"PRIpaddr" new %lx\n",
                  nvmx->vmxon_region_pa, gpa);
@@ -1417,7 +1429,7 @@ int nvmx_handle_vmxoff(struct cpu_user_regs *regs)
         return rc;
 
     nvmx_purge_vvmcs(v);
-    nvmx->vmxon_region_pa = 0;
+    nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
 
     vmreturn(regs, VMSUCCEED);
     return X86EMUL_OKAY;
-- 
2.10.1


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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Andrew Cooper]

On 13/12/16 12:16, Haozhong Zhang wrote:
> nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
> operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
> also a valid VMXON region address. If L1 hypervisor had set the VMXON
> region address to 0, the check in nvmx_handle_vmxon() will be skipped.
> Fix this problem by using an invalid VMXON region address for vcpu
> out of VMX operation.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index e6e9ebd..f5637eb 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>  
>  static void nvmx_purge_vvmcs(struct vcpu *v);
>  
> +/*
> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> + * VMXON region address.
> + */
> +#define INVALID_VMXON_REGION_PA (~0UL)
> +
> +static bool nvmx_vcpu_in_vmx(struct vcpu *v)

const struct vcpu *v.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I can fix this up on commit if no other issues are found by the other
reviewers.

> +{
> +    return vcpu_2_nvmx(v).vmxon_region_pa != INVALID_VMXON_REGION_PA;
> +}
> +
>  #define VMCS_BUF_SIZE 100
>  
>  int nvmx_cpu_up_prepare(unsigned int cpu)
> @@ -107,7 +119,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
>  
>      nvmx->ept.enabled = 0;
>      nvmx->guest_vpid = 0;
> -    nvmx->vmxon_region_pa = 0;
> +    nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
>      nvcpu->nv_vvmcx = NULL;
>      nvcpu->nv_vvmcxaddr = VMCX_EADDR;
>      nvmx->intr.intr_info = 0;
> @@ -357,7 +369,7 @@ static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
>               !(v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_VMXE) )
>              goto invalid_op;
>      }
> -    else if ( !vcpu_2_nvmx(v).vmxon_region_pa )
> +    else if ( !nvmx_vcpu_in_vmx(v) )
>          goto invalid_op;
>  
>      hvm_get_segment_register(v, x86_seg_cs, &cs);
> @@ -1384,7 +1396,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>      if ( rc != X86EMUL_OKAY )
>          return rc;
>  
> -    if ( nvmx->vmxon_region_pa )
> +    if ( nvmx_vcpu_in_vmx(v) )
>          gdprintk(XENLOG_WARNING, 
>                   "vmxon again: orig %"PRIpaddr" new %lx\n",
>                   nvmx->vmxon_region_pa, gpa);
> @@ -1417,7 +1429,7 @@ int nvmx_handle_vmxoff(struct cpu_user_regs *regs)
>          return rc;
>  
>      nvmx_purge_vvmcs(v);
> -    nvmx->vmxon_region_pa = 0;
> +    nvmx->vmxon_region_pa = INVALID_VMXON_REGION_PA;
>  
>      vmreturn(regs, VMSUCCEED);
>      return X86EMUL_OKAY;


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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Konrad Rzeszutek Wilk]

On Tue, Dec 13, 2016 at 02:35:33PM +0000, Andrew Cooper wrote:
> On 13/12/16 12:16, Haozhong Zhang wrote:
> > nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
> > operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
> > also a valid VMXON region address. If L1 hypervisor had set the VMXON
> > region address to 0, the check in nvmx_handle_vmxon() will be skipped.
> > Fix this problem by using an invalid VMXON region address for vcpu
> > out of VMX operation.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  xen/arch/x86/hvm/vmx/vvmx.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index e6e9ebd..f5637eb 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
> >  
> >  static void nvmx_purge_vvmcs(struct vcpu *v);
> >  
> > +/*
> > + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> > + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> > + * VMXON region address.
> > + */
> > +#define INVALID_VMXON_REGION_PA (~0UL)
> > +
> > +static bool nvmx_vcpu_in_vmx(struct vcpu *v)
> 
> const struct vcpu *v.
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Jan Beulich]

>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>  
>  static void nvmx_purge_vvmcs(struct vcpu *v);
>  
> +/*
> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> + * VMXON region address.
> + */
> +#define INVALID_VMXON_REGION_PA (~0UL)

vmxon_region_pa being paddr_t, I think this would better be
explicitly paddr_t, too (rather than unsigned long).

Jan


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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Jan Beulich]

>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>  
>  static void nvmx_purge_vvmcs(struct vcpu *v);
>  
> +/*
> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
> + * VMXON region address.
> + */
> +#define INVALID_VMXON_REGION_PA (~0UL)

And btw, having looked at patch 2 - any reason you can't simply
re-use VMCX_EADDR here?

Jan


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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Haozhong Zhang]

On 12/13/16 08:21 -0700, Jan Beulich wrote:
>>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>>
>>  static void nvmx_purge_vvmcs(struct vcpu *v);
>>
>> +/*
>> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
>> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
>> + * VMXON region address.
>> + */
>> +#define INVALID_VMXON_REGION_PA (~0UL)
>
>And btw, having looked at patch 2 - any reason you can't simply
>re-use VMCX_EADDR here?
>

I just find INVALID_PADDR defined along with type paddr_t:

typedef unsigned long paddr_t;
#define INVALID_PADDR (~0UL)

Which one, INVALID_PADDR or VMCX_EADDR, would be better?

Thanks,
Haozhong

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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Jan Beulich]

>>> On 14.12.16 at 02:37, <haozhong.zhang@intel.com> wrote:
> On 12/13/16 08:21 -0700, Jan Beulich wrote:
>>>>> On 13.12.16 at 13:16, <haozhong.zhang@intel.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -32,6 +32,18 @@ static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>>>
>>>  static void nvmx_purge_vvmcs(struct vcpu *v);
>>>
>>> +/*
>>> + * When a vcpu is out of VMXON region, set its vmxon_region_pa to
>>> + * INVALID_VMXON_REGION_PA. We cannot use 0, because 0 is also a valid
>>> + * VMXON region address.
>>> + */
>>> +#define INVALID_VMXON_REGION_PA (~0UL)
>>
>>And btw, having looked at patch 2 - any reason you can't simply
>>re-use VMCX_EADDR here?
>>
> 
> I just find INVALID_PADDR defined along with type paddr_t:
> 
> typedef unsigned long paddr_t;
> #define INVALID_PADDR (~0UL)
> 
> Which one, INVALID_PADDR or VMCX_EADDR, would be better?

The former then, I would say. And perhaps ditch VMCX_EADDR in
a separate patch then too, in favor of the more general one.

Jan


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

Re: [PATCH 1/3] vvmx: set vmxon_region_pa of vcpu out of VMX operation to an invalid address

[Tian, Kevin]

> From: Zhang, Haozhong
> Sent: Tuesday, December 13, 2016 8:16 PM
> 
> nvmx_handle_vmxon() previously checks whether a vcpu is in VMX
> operation by comparing its vmxon_region_pa with GPA 0. However, 0 is
> also a valid VMXON region address. If L1 hypervisor had set the VMXON
> region address to 0, the check in nvmx_handle_vmxon() will be skipped.
> Fix this problem by using an invalid VMXON region address for vcpu
> out of VMX operation.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

once you have type thing fixed:

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

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

[PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation

[Haozhong Zhang]

According to Intel SDM, section "VMXON - Enter VMX Operation", a
VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
VMX operation, rather than just print a warning message.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index f5637eb..b60d7f0 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1397,9 +1397,12 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
         return rc;
 
     if ( nvmx_vcpu_in_vmx(v) )
-        gdprintk(XENLOG_WARNING, 
-                 "vmxon again: orig %"PRIpaddr" new %lx\n",
-                 nvmx->vmxon_region_pa, gpa);
+    {
+        vmreturn(regs,
+                 nvcpu->nv_vvmcxaddr != VMCX_EADDR ?
+                 VMFAIL_VALID : VMFAIL_INVALID);
+        return X86EMUL_OKAY;
+    }
 
     nvmx->vmxon_region_pa = gpa;
 
-- 
2.10.1


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

Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation

[Andrew Cooper]

On 13/12/16 12:16, Haozhong Zhang wrote:
> According to Intel SDM, section "VMXON - Enter VMX Operation", a
> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
> VMX operation, rather than just print a warning message.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although

> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index f5637eb..b60d7f0 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1397,9 +1397,12 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>          return rc;
>  
>      if ( nvmx_vcpu_in_vmx(v) )
> -        gdprintk(XENLOG_WARNING, 
> -                 "vmxon again: orig %"PRIpaddr" new %lx\n",
> -                 nvmx->vmxon_region_pa, gpa);
> +    {
> +        vmreturn(regs,
> +                 nvcpu->nv_vvmcxaddr != VMCX_EADDR ?
> +                 VMFAIL_VALID : VMFAIL_INVALID);

vmreturn really should take a single fail option and internally turn
this into VMFAIL_{VALID,INVALID}, rather than requiring the caller to do so.

~Andrew

> +        return X86EMUL_OKAY;
> +    }
>  
>      nvmx->vmxon_region_pa = gpa;
>  


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

Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation

[Konrad Rzeszutek Wilk]

On Tue, Dec 13, 2016 at 08:16:19PM +0800, Haozhong Zhang wrote:
> According to Intel SDM, section "VMXON - Enter VMX Operation", a
> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
> VMX operation, rather than just print a warning message.

The spec also says to return value 15? But I suppose that means
the TOOD in vmreturn should be implemented?

> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index f5637eb..b60d7f0 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1397,9 +1397,12 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>          return rc;
>  
>      if ( nvmx_vcpu_in_vmx(v) )
> -        gdprintk(XENLOG_WARNING, 
> -                 "vmxon again: orig %"PRIpaddr" new %lx\n",
> -                 nvmx->vmxon_region_pa, gpa);
> +    {
> +        vmreturn(regs,
> +                 nvcpu->nv_vvmcxaddr != VMCX_EADDR ?
> +                 VMFAIL_VALID : VMFAIL_INVALID);
> +        return X86EMUL_OKAY;
> +    }
>  
>      nvmx->vmxon_region_pa = gpa;
>  
> -- 
> 2.10.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation

[Haozhong Zhang]

On 12/13/16 10:16 -0500, Konrad Rzeszutek Wilk wrote:
>On Tue, Dec 13, 2016 at 08:16:19PM +0800, Haozhong Zhang wrote:
>> According to Intel SDM, section "VMXON - Enter VMX Operation", a
>> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
>> VMX operation, rather than just print a warning message.
>
>The spec also says to return value 15? But I suppose that means
>the TOOD in vmreturn should be implemented?
>

Yes, it's on my TODO list.

Haozhong

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

Re: [PATCH 2/3] vvmx: return VMfail to L1 if L1 vmxon is executed in VMX operation

[Tian, Kevin]

> From: Zhang, Haozhong
> Sent: Tuesday, December 13, 2016 8:16 PM
> 
> According to Intel SDM, section "VMXON - Enter VMX Operation", a
> VMfail should be returned to L1 hypervisor if L1 vmxon is executed in
> VMX operation, rather than just print a warning message.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

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

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

[PATCH 3/3] vvmx: check the operand of L1 vmxon

[Haozhong Zhang]

Check whether the operand of L1 vmxon is a valid VMXON region address
and whether the VMXON region at that address contains a valid revision
ID.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index b60d7f0..7cee307 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1390,6 +1390,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     struct vmx_inst_decoded decode;
     unsigned long gpa = 0;
+    uint32_t nvmcs_revid;
     int rc;
 
     rc = decode_vmx_inst(regs, &decode, &gpa, 1);
@@ -1404,6 +1405,21 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
+    if ( (gpa & ~PAGE_MASK) || (gpa >> v->domain->arch.paging.gfn_bits) )
+    {
+        vmreturn(regs, VMFAIL_INVALID);
+        return X86EMUL_OKAY;
+    }
+
+    rc = hvm_copy_from_guest_phys(&nvmcs_revid, gpa, sizeof(nvmcs_revid));
+    if ( rc != HVMCOPY_okay ||
+         (nvmcs_revid & ~VMX_BASIC_REVISION_MASK) ||
+         ((nvmcs_revid ^ vmx_basic_msr) & VMX_BASIC_REVISION_MASK) )
+    {
+        vmreturn(regs, VMFAIL_INVALID);
+        return X86EMUL_OKAY;
+    }
+
     nvmx->vmxon_region_pa = gpa;
 
     /*
-- 
2.10.1


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

Re: [PATCH 3/3] vvmx: check the operand of L1 vmxon

[Andrew Cooper]

On 13/12/16 12:16, Haozhong Zhang wrote:
> Check whether the operand of L1 vmxon is a valid VMXON region address
> and whether the VMXON region at that address contains a valid revision
> ID.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 3/3] vvmx: check the operand of L1 vmxon

[Konrad Rzeszutek Wilk]

On Tue, Dec 13, 2016 at 08:16:20PM +0800, Haozhong Zhang wrote:
> Check whether the operand of L1 vmxon is a valid VMXON region address
> and whether the VMXON region at that address contains a valid revision
> ID.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you finding these and fixing them!

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

Re: [PATCH 3/3] vvmx: check the operand of L1 vmxon

[Tian, Kevin]

> From: Zhang, Haozhong
> Sent: Tuesday, December 13, 2016 8:16 PM
> 
> Check whether the operand of L1 vmxon is a valid VMXON region address
> and whether the VMXON region at that address contains a valid revision
> ID.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

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

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