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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 14:44, <sergey.dyasli@xxxxxxxxxx> wrote:
> On Wed, 2017-03-01 at 05:55 -0700, Jan Beulich wrote:
>> > > > On 01.03.17 at 10:13, <sergey.dyasli@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel