| Message ID | 20170301091355.24742-2-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
>>> 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
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
>>> 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
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
>>> 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
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
>>> 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
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
>>> 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
> 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
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 )
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(+)