[PATCH v1 0/2] x86/vvmx: a couple of fixes (vmptrld + vmread/vmwrite)

[PATCH v1 0/2] x86/vvmx: a couple of fixes (vmptrld + vmread/vmwrite)

[Sergey Dyasli]

I have found 2 more ways to crash Xen from a guest with nested virt.
These patches implement some missing checks.

Sergey Dyasli (2):
  x86/vvmx: check vmcs address in vmread/vmwrite
  x86/vvmx: add vmcs id check into vmptrld emulation

 xen/arch/x86/hvm/vmx/vvmx.c        | 23 +++++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
 2 files changed, 24 insertions(+)

-- 
2.9.3


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

[PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Sergey Dyasli]

If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
during vmread/vmwrite:

(XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
(XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
(XEN) Xen call trace:
(XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
(XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
(XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
(XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
(XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120

Fix this by emulating VMfailInvalid if the address is invalid.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index f6a25a6..4f5ee5a 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1751,6 +1751,12 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
+    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+    {
+        vmfail_invalid(regs);
+        return X86EMUL_OKAY;
+    }
+
     rc = get_vvmcs_safe(v, reg_read(regs, decode.reg2), &value);
     if ( rc != VMX_INSN_SUCCEED )
     {
@@ -1788,6 +1794,12 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
              != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
+    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+    {
+        vmfail_invalid(regs);
+        return X86EMUL_OKAY;
+    }
+
     vmcs_encoding = reg_read(regs, decode.reg2);
     err = set_vvmcs_safe(v, vmcs_encoding, operand);
     if ( err != VMX_INSN_SUCCEED )
-- 
2.9.3


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

[PATCH v1 2/2] x86/vvmx: add vmcs id check into vmptrld emulation

[Sergey Dyasli]

If a guest will do vmptrld with an incorrect vmcs id:

(XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
(XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
(XEN) Xen call trace:
(XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
(XEN)    [<ffff82d0801f602c>] virtual_vmcs_vmread+0x11/0x2c
(XEN)    [<ffff82d0802002cc>] vvmx.c#_map_io_bitmap+0x86/0x88
(XEN)    [<ffff82d080202399>] nvmx_handle_vmptrld+0xf0/0x1fb
(XEN)    [<ffff82d0801fe93c>] vmx_vmexit_handler+0x132b/0x1c49
(XEN)    [<ffff82d080203e6c>] vmx_asm_vmexit_handler+0x3c/0x120

Fix this by adding appropriate checks for vmcs id during vmptrld
emulation.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c        | 11 +++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 4f5ee5a..580fd3e 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1633,6 +1633,17 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
         {
             if ( writable )
             {
+                struct vmcs_struct *vvmcs = vvmcx;
+
+                if ( ((vvmcs->vmcs_revision_id ^ vmx_basic_msr) &
+                                         VMX_BASIC_REVISION_MASK) ||
+                     (!cpu_has_vmx_vmcs_shadowing &&
+                      (vvmcs->vmcs_revision_id & ~VMX_BASIC_REVISION_MASK)) )
+                {
+                    hvm_unmap_guest_frame(vvmcx, 1);
+                    vmfail(regs, VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID);
+                    return X86EMUL_OKAY;
+                }
                 nvcpu->nv_vvmcx = vvmcx;
                 nvcpu->nv_vvmcxaddr = gpa;
                 v->arch.hvm_vmx.vmcs_shadow_maddr =
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 4ee01da..6308e4b 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -521,6 +521,7 @@ enum vmx_insn_errno
     VMX_INSN_INVALID_CONTROL_STATE         = 7,
     VMX_INSN_INVALID_HOST_STATE            = 8,
     VMX_INSN_VMPTRLD_INVALID_PHYADDR       = 9,
+    VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID     = 11,
     VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
     VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
     VMX_INSN_FAIL_INVALID                  = ~0,
-- 
2.9.3


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Andrew Cooper]

On 01/03/17 09:13, Sergey Dyasli wrote:
> If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
> during vmread/vmwrite:
>
> (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
> (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
> (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
> (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
> (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
> (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
>
> Fix this by emulating VMfailInvalid if the address is invalid.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.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 v1 2/2] x86/vvmx: add vmcs id check into vmptrld emulation

[Andrew Cooper]

On 01/03/17 09:13, Sergey Dyasli wrote:
> If a guest will do vmptrld with an incorrect vmcs id:
>
> (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
> (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
> (XEN)    [<ffff82d0801f602c>] virtual_vmcs_vmread+0x11/0x2c
> (XEN)    [<ffff82d0802002cc>] vvmx.c#_map_io_bitmap+0x86/0x88
> (XEN)    [<ffff82d080202399>] nvmx_handle_vmptrld+0xf0/0x1fb
> (XEN)    [<ffff82d0801fe93c>] vmx_vmexit_handler+0x132b/0x1c49
> (XEN)    [<ffff82d080203e6c>] vmx_asm_vmexit_handler+0x3c/0x120
>
> Fix this by adding appropriate checks for vmcs id during vmptrld
> emulation.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c        | 11 +++++++++++
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 4f5ee5a..580fd3e 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1633,6 +1633,17 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
>          {
>              if ( writable )
>              {
> +                struct vmcs_struct *vvmcs = vvmcx;
> +
> +                if ( ((vvmcs->vmcs_revision_id ^ vmx_basic_msr) &
> +                                         VMX_BASIC_REVISION_MASK) ||
> +                     (!cpu_has_vmx_vmcs_shadowing &&
> +                      (vvmcs->vmcs_revision_id & ~VMX_BASIC_REVISION_MASK)) )
> +                {
> +                    hvm_unmap_guest_frame(vvmcx, 1);
> +                    vmfail(regs, VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID);

A newline here please (can be fixed on commit if there are no other
issues).  Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +                    return X86EMUL_OKAY;
> +                }
>                  nvcpu->nv_vvmcx = vvmcx;
>                  nvcpu->nv_vvmcxaddr = gpa;
>                  v->arch.hvm_vmx.vmcs_shadow_maddr =
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 4ee01da..6308e4b 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -521,6 +521,7 @@ enum vmx_insn_errno
>      VMX_INSN_INVALID_CONTROL_STATE         = 7,
>      VMX_INSN_INVALID_HOST_STATE            = 8,
>      VMX_INSN_VMPTRLD_INVALID_PHYADDR       = 9,
> +    VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID     = 11,
>      VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
>      VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
>      VMX_INSN_FAIL_INVALID                  = ~0,


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Jan Beulich]

>>> On 01.03.17 at 10:13, <sergey.dyasli@citrix.com> wrote:
> If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
> during vmread/vmwrite:
> 
> (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
> (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
> (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
> (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
> (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
> (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
> 
> Fix this by emulating VMfailInvalid if the address is invalid.

So just like in patch 2 this is __vmptrld() not properly dealing with
errors. Instead of doing checks in software which hardware does
anyway, wouldn't it be better to introduce (and use here and
there) vmptrld_safe()?

Jan


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Andrew Cooper]

On 01/03/17 12:55, Jan Beulich wrote:
>>>> On 01.03.17 at 10:13, <sergey.dyasli@citrix.com> wrote:
>> If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
>> during vmread/vmwrite:
>>
>> (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
>> (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
>> (XEN) Xen call trace:
>> (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
>> (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
>> (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
>> (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
>> (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
>>
>> Fix this by emulating VMfailInvalid if the address is invalid.
> So just like in patch 2 this is __vmptrld() not properly dealing with
> errors. Instead of doing checks in software which hardware does
> anyway, wouldn't it be better to introduce (and use here and
> there) vmptrld_safe()?

Lonterm, I'd like to see us move to a model where the base version is
safe, and error handling is along the lines of:

if ( on_behalf_of_nested )
    propagate_to_guest();
else
{
    dump_relevent_info();
    domain_crash(current->domain);
}


This is far more useful for error isolation (not taking the host down
even if it is Xen's fault the failure occured), and diagnosing what
actually went wrong, than simply having hit a BUG().

~Andrew

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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Jan Beulich]

>>> On 01.03.17 at 14:22, <andrew.cooper3@citrix.com> wrote:
> On 01/03/17 12:55, Jan Beulich wrote:
>>>>> On 01.03.17 at 10:13, <sergey.dyasli@citrix.com> wrote:
>>> If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
>>> during vmread/vmwrite:
>>>
>>> (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
>>> (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
>>> (XEN) Xen call trace:
>>> (XEN)    [<ffff82d0801f925e>] 
> vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
>>> (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
>>> (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
>>> (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
>>> (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
>>>
>>> Fix this by emulating VMfailInvalid if the address is invalid.
>> So just like in patch 2 this is __vmptrld() not properly dealing with
>> errors. Instead of doing checks in software which hardware does
>> anyway, wouldn't it be better to introduce (and use here and
>> there) vmptrld_safe()?
> 
> Lonterm, I'd like to see us move to a model where the base version is
> safe, and error handling is along the lines of:
> 
> if ( on_behalf_of_nested )
>     propagate_to_guest();
> else
> {
>     dump_relevent_info();
>     domain_crash(current->domain);
> }
> 
> 
> This is far more useful for error isolation (not taking the host down
> even if it is Xen's fault the failure occured), and diagnosing what
> actually went wrong, than simply having hit a BUG().

I completely agree, but I don't think you or I mean to ask Sergey to
do the full conversion right away. I.e. I think your reply is somewhat
orthogonal to (or is going way beyond) my request.

Jan


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Sergey Dyasli]

On Wed, 2017-03-01 at 05:55 -0700, Jan Beulich wrote:
> > > > On 01.03.17 at 10:13, <sergey.dyasli@citrix.com> wrote:
> > 
> > If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
> > during vmread/vmwrite:
> > 
> > (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
> > (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
> > (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
> > (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
> > (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
> > (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
> > 
> > Fix this by emulating VMfailInvalid if the address is invalid.
> 
> So just like in patch 2 this is __vmptrld() not properly dealing with
> errors. Instead of doing checks in software which hardware does
> anyway, wouldn't it be better to introduce (and use here and
> there) vmptrld_safe()?

Currently it's assumed that virtual_vmcs_enter/exit() never fail.
It's easy to maintain that assumption with one simple check:

    nv_vvmcxaddr != INVALID_PADDR

as long as nvmx_handle_vmptrld() correctly checks the validity of
provided pointer.

Additionally, it would be painful to return the correct error value
all the way back to nvmx_handle_vmptrld().

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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Jan Beulich]

>>> On 01.03.17 at 14:44, <sergey.dyasli@citrix.com> wrote:
> On Wed, 2017-03-01 at 05:55 -0700, Jan Beulich wrote:
>> > > > On 01.03.17 at 10:13, <sergey.dyasli@citrix.com> wrote:
>> > 
>> > If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
>> > during vmread/vmwrite:
>> > 
>> > (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
>> > (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
>> > (XEN) Xen call trace:
>> > (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
>> > (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
>> > (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
>> > (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
>> > (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
>> > 
>> > Fix this by emulating VMfailInvalid if the address is invalid.
>> 
>> So just like in patch 2 this is __vmptrld() not properly dealing with
>> errors. Instead of doing checks in software which hardware does
>> anyway, wouldn't it be better to introduce (and use here and
>> there) vmptrld_safe()?
> 
> Currently it's assumed that virtual_vmcs_enter/exit() never fail.
> It's easy to maintain that assumption with one simple check:
> 
>     nv_vvmcxaddr != INVALID_PADDR
> 
> as long as nvmx_handle_vmptrld() correctly checks the validity of
> provided pointer.

Yet even more safe would be to avoid the check here and simply
properly check and convey the instruction results.

> Additionally, it would be painful to return the correct error value
> all the way back to nvmx_handle_vmptrld().

Surely that's at best relevant in the other patch. Here you're in
virtual_vmcs_vmwrite_safe(), which already knows how to
communicate back an error indicator.

Jan


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Sergey Dyasli]

On Wed, 2017-03-01 at 07:04 -0700, Jan Beulich wrote:
> > > > On 01.03.17 at 14:44, <sergey.dyasli@citrix.com> wrote:
> > 
> > On Wed, 2017-03-01 at 05:55 -0700, Jan Beulich wrote:
> > > > > > On 01.03.17 at 10:13, <sergey.dyasli@citrix.com> wrote:
> > > > 
> > > > If nested vmcs's address is invalid, virtual_vmcs_enter() will fail
> > > > during vmread/vmwrite:
> > > > 
> > > > (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
> > > > (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
> > > > (XEN) Xen call trace:
> > > > (XEN)    [<ffff82d0801f925e>] vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
> > > > (XEN)    [<ffff82d0801f60e3>] virtual_vmcs_vmwrite_safe+0x16/0x52
> > > > (XEN)    [<ffff82d080202cb2>] nvmx_handle_vmwrite+0x70/0xfe
> > > > (XEN)    [<ffff82d0801fe98a>] vmx_vmexit_handler+0x1379/0x1c49
> > > > (XEN)    [<ffff82d08020427c>] vmx_asm_vmexit_handler+0x3c/0x120
> > > > 
> > > > Fix this by emulating VMfailInvalid if the address is invalid.
> > > 
> > > So just like in patch 2 this is __vmptrld() not properly dealing with
> > > errors. Instead of doing checks in software which hardware does
> > > anyway, wouldn't it be better to introduce (and use here and
> > > there) vmptrld_safe()?
> > 
> > Currently it's assumed that virtual_vmcs_enter/exit() never fail.
> > It's easy to maintain that assumption with one simple check:
> > 
> >     nv_vvmcxaddr != INVALID_PADDR
> > 
> > as long as nvmx_handle_vmptrld() correctly checks the validity of
> > provided pointer.
> 
> Yet even more safe would be to avoid the check here and simply
> properly check and convey the instruction results.

In the future it should be possible to limit (level) the set of VMX
features provided to the guest.  One example would be VMCS shadowing.
In such cases checks mustn't be done by H/W since it can be different.

> > Additionally, it would be painful to return the correct error value
> > all the way back to nvmx_handle_vmptrld().
> 
> Surely that's at best relevant in the other patch. Here you're in
> virtual_vmcs_vmwrite_safe(), which already knows how to
> communicate back an error indicator.

How checking the return value of virtual_vmcs_enter() is different
from checking nv_vvmcxaddr?

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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Jan Beulich]

>>> On 01.03.17 at 15:22, <sergey.dyasli@citrix.com> wrote:
> On Wed, 2017-03-01 at 07:04 -0700, Jan Beulich wrote:
>> > > > On 01.03.17 at 14:44, <sergey.dyasli@citrix.com> wrote:
>> > Additionally, it would be painful to return the correct error value
>> > all the way back to nvmx_handle_vmptrld().
>> 
>> Surely that's at best relevant in the other patch. Here you're in
>> virtual_vmcs_vmwrite_safe(), which already knows how to
>> communicate back an error indicator.
> 
> How checking the return value of virtual_vmcs_enter() is different
> from checking nv_vvmcxaddr?

Checking the address is just one step. As the other patch shows,
checking the ID is necessary too. Over time more such checks may
be found necessary. Checking what hardware hands us is a single
check, and is always going to be sufficient no matter what new
features get added to hardware.

Jan


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Sergey Dyasli]

On Wed, 2017-03-01 at 07:28 -0700, Jan Beulich wrote:
> > > > On 01.03.17 at 15:22, <sergey.dyasli@citrix.com> wrote:
> > 
> > On Wed, 2017-03-01 at 07:04 -0700, Jan Beulich wrote:
> > > > > > On 01.03.17 at 14:44, <sergey.dyasli@citrix.com> wrote:
> > > > 
> > > > Additionally, it would be painful to return the correct error value
> > > > all the way back to nvmx_handle_vmptrld().
> > > 
> > > Surely that's at best relevant in the other patch. Here you're in
> > > virtual_vmcs_vmwrite_safe(), which already knows how to
> > > communicate back an error indicator.
> > 
> > How checking the return value of virtual_vmcs_enter() is different
> > from checking nv_vvmcxaddr?
> 
> Checking the address is just one step. As the other patch shows,
> checking the ID is necessary too. Over time more such checks may
> be found necessary. Checking what hardware hands us is a single
> check, and is always going to be sufficient no matter what new
> features get added to hardware.

Conceptually, the result of __vmptrld() is irrelevant to a guest
during nested vmread/vmwrite emulation.  The fact that Xen is doing
__vmptrld() is a side effect of nested VMX.

All necessary checks may be found in Intel SDM.  And it states that
there can be only 1 VMfail if VMCS pointer is not valid: VMfailInvalid.
vmptrld can end up in 3 different VMfails and returning them to the
guest as a result of vmread/vmwrite emulation is wrong.

(Even though each of them will end up being VMfailInvalid in current
implementation)

I think that Xen's goal in nested VMX is emulating H/W as close as
possible.

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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Jan Beulich]

>>> On 01.03.17 at 16:23, <sergey.dyasli@citrix.com> wrote:
> On Wed, 2017-03-01 at 07:28 -0700, Jan Beulich wrote:
>> > > > On 01.03.17 at 15:22, <sergey.dyasli@citrix.com> wrote:
>> > 
>> > On Wed, 2017-03-01 at 07:04 -0700, Jan Beulich wrote:
>> > > > > > On 01.03.17 at 14:44, <sergey.dyasli@citrix.com> wrote:
>> > > > 
>> > > > Additionally, it would be painful to return the correct error value
>> > > > all the way back to nvmx_handle_vmptrld().
>> > > 
>> > > Surely that's at best relevant in the other patch. Here you're in
>> > > virtual_vmcs_vmwrite_safe(), which already knows how to
>> > > communicate back an error indicator.
>> > 
>> > How checking the return value of virtual_vmcs_enter() is different
>> > from checking nv_vvmcxaddr?
>> 
>> Checking the address is just one step. As the other patch shows,
>> checking the ID is necessary too. Over time more such checks may
>> be found necessary. Checking what hardware hands us is a single
>> check, and is always going to be sufficient no matter what new
>> features get added to hardware.
> 
> Conceptually, the result of __vmptrld() is irrelevant to a guest
> during nested vmread/vmwrite emulation.  The fact that Xen is doing
> __vmptrld() is a side effect of nested VMX.

True.

> All necessary checks may be found in Intel SDM.

All _currently_ necessary checks. It is precisely possible new ones
getting added which I'd like to see covered by other than adding
further checks to our software when hardware already does them.

>  And it states that
> there can be only 1 VMfail if VMCS pointer is not valid: VMfailInvalid.
> vmptrld can end up in 3 different VMfails and returning them to the
> guest as a result of vmread/vmwrite emulation is wrong.

As is crashing Xen because of such.

The implication of course would be that the insn-error may need
adjustment in such a case.

> (Even though each of them will end up being VMfailInvalid in current
> implementation)
> 
> I think that Xen's goal in nested VMX is emulating H/W as close as
> possible.

Correct. Preferably by leveraging hardware instead of re-doing the
same thing in software.

Anyway - we'll see what the VMX maintainers think.

Jan


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

Re: [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, March 01, 2017 11:40 PM
> 
> >>> On 01.03.17 at 16:23, <sergey.dyasli@citrix.com> wrote:
> > On Wed, 2017-03-01 at 07:28 -0700, Jan Beulich wrote:
> >> > > > On 01.03.17 at 15:22, <sergey.dyasli@citrix.com> wrote:
> >> >
> >> > On Wed, 2017-03-01 at 07:04 -0700, Jan Beulich wrote:
> >> > > > > > On 01.03.17 at 14:44, <sergey.dyasli@citrix.com> wrote:
> >> > > >
> >> > > > Additionally, it would be painful to return the correct error value
> >> > > > all the way back to nvmx_handle_vmptrld().
> >> > >
> >> > > Surely that's at best relevant in the other patch. Here you're in
> >> > > virtual_vmcs_vmwrite_safe(), which already knows how to
> >> > > communicate back an error indicator.
> >> >
> >> > How checking the return value of virtual_vmcs_enter() is different
> >> > from checking nv_vvmcxaddr?
> >>
> >> Checking the address is just one step. As the other patch shows,
> >> checking the ID is necessary too. Over time more such checks may
> >> be found necessary. Checking what hardware hands us is a single
> >> check, and is always going to be sufficient no matter what new
> >> features get added to hardware.
> >
> > Conceptually, the result of __vmptrld() is irrelevant to a guest
> > during nested vmread/vmwrite emulation.  The fact that Xen is doing
> > __vmptrld() is a side effect of nested VMX.
> 
> True.
> 
> > All necessary checks may be found in Intel SDM.
> 
> All _currently_ necessary checks. It is precisely possible new ones
> getting added which I'd like to see covered by other than adding
> further checks to our software when hardware already does them.
> 
> >  And it states that
> > there can be only 1 VMfail if VMCS pointer is not valid: VMfailInvalid.
> > vmptrld can end up in 3 different VMfails and returning them to the
> > guest as a result of vmread/vmwrite emulation is wrong.
> 
> As is crashing Xen because of such.
> 
> The implication of course would be that the insn-error may need
> adjustment in such a case.
> 
> > (Even though each of them will end up being VMfailInvalid in current
> > implementation)
> >
> > I think that Xen's goal in nested VMX is emulating H/W as close as
> > possible.
> 
> Correct. Preferably by leveraging hardware instead of re-doing the
> same thing in software.
> 
> Anyway - we'll see what the VMX maintainers think.
> 

Although leveraging HW check is a generally good idea, I buy-in 
Sergey's comment that we may emulate different VMX feature set 
to guest in the future then in such case we'll need both SW/HW 
checks and then may still need to track latest SDM change.

So:

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

Thanks
Kevin

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

Re: [PATCH v1 2/2] x86/vvmx: add vmcs id check into vmptrld emulation

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Wednesday, March 01, 2017 5:14 PM
> 
> If a guest will do vmptrld with an incorrect vmcs id:
> 
> (XEN) Xen BUG at .../git/upstream/xen/xen/include/asm/hvm/vmx/vmx.h:333
> (XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Tainted:    H ]----
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0801f925e>]
> vmcs.c#arch/x86/hvm/vmx/vmcs.o.unlikely+0x28/0x19a
> (XEN)    [<ffff82d0801f602c>] virtual_vmcs_vmread+0x11/0x2c
> (XEN)    [<ffff82d0802002cc>] vvmx.c#_map_io_bitmap+0x86/0x88
> (XEN)    [<ffff82d080202399>] nvmx_handle_vmptrld+0xf0/0x1fb
> (XEN)    [<ffff82d0801fe93c>] vmx_vmexit_handler+0x132b/0x1c49
> (XEN)    [<ffff82d080203e6c>] vmx_asm_vmexit_handler+0x3c/0x120
> 
> Fix this by adding appropriate checks for vmcs id during vmptrld
> emulation.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

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

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

Re: [PATCH v1 2/2] x86/vvmx: add vmcs id check into vmptrld emulation

[Jan Beulich]

>>> On 01.03.17 at 12:01, <andrew.cooper3@citrix.com> wrote:
> On 01/03/17 09:13, Sergey Dyasli wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -1633,6 +1633,17 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
>>          {
>>              if ( writable )
>>              {
>> +                struct vmcs_struct *vvmcs = vvmcx;
>> +
>> +                if ( ((vvmcs->vmcs_revision_id ^ vmx_basic_msr) &
>> +                                         VMX_BASIC_REVISION_MASK) ||
>> +                     (!cpu_has_vmx_vmcs_shadowing &&
>> +                      (vvmcs->vmcs_revision_id & ~VMX_BASIC_REVISION_MASK)) )
>> +                {
>> +                    hvm_unmap_guest_frame(vvmcx, 1);
>> +                    vmfail(regs, VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID);
> 
> A newline here please (can be fixed on commit if there are no other
> issues).  Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
>> +                    return X86EMUL_OKAY;
>> +                }

I've added one, but commonly we require such only on the main
(final) return statement of a function.

Jan


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