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

[Sergey Dyasli <sergey.dyasli@citrix.com>]

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