Re: [PATCH V5] x86/altp2m: Fix crash with INVALID_ALTP2M EPTP index

[George Dunlap <george.dunlap@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

On 06/28/2018 03:35 PM, Razvan Cojocaru wrote:
> A VM exit handler executed immediately after enabling #VE might
> find a stale __vmsave()d EPTP_INDEX, stored by calling
> altp2m_vcpu_destroy() when SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS
> had been enabled by altp2m_vcpu_update_vmfunc_ve().
> 
> vmx_vmexit_handler() __vmread()s EPTP_INDEX as soon as
> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set, so if an
> application enables altp2m on a domain, succesfully calls
> xc_altp2m_set_vcpu_enable_notify(), then disables altp2m and
> exits, a second run of said application will likely read the
> INVALID_ALTP2M EPTP_INDEX set when disabling altp2m in the first
> run, and crash the host with the BUG_ON(idx >= MAX_ALTP2M),
> between xc_altp2m_set_vcpu_enable_notify() and
> xc_altp2m_set_domain_state(..., false).
> 
> The problem is not restricted to an INVALID_ALTP2M EPTP_INDEX
> (which can only sanely happen on altp2m uninit), but applies
> to any stale index previously saved - which means that all
> altp2m_vcpu_update_vmfunc_ve() calls must also call
> altp2m_vcpu_update_p2m() after setting
> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS, in order to make sure
> that the stored EPTP_INDEX is always valid at
> vmx_vmexit_handler() time.

I'm sorry, this description still doesn't make hardly any sense to me,
nor the solution, even after reading all the previous threads on the
issue.  The description doesn't, for instance, mention vcpu_pause() at
all, in spite of the fact that it seems (from the previous discussion)
that this is a critical part of why this solution works; nor is there
any comment in the code about the required discipline regarding
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS,  making it fairly likely that
someone will re-introduce a bug like this in the future.

My normal template for something like this is
1. Explain what the current situation is
2. Explain why that's a problem
3. Describe what you're changing and how it fixes it.

I can't help but think the right thing to do here is in vmx.c somewhere
-- it is, after all, code in vmx.c that:
1. Sets and clears SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS
2. Writes EPTP_INDEX
3. Assumes that SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS => EPTP_INDEX is
valid.

What about something like the attached, instead (compile-tested only)?

 -George

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-x86-altp2m-Make-sure-EPTP_INDEX-is-up-to-date-when-e.patch --]
[-- Type: text/x-patch; name="0001-x86-altp2m-Make-sure-EPTP_INDEX-is-up-to-date-when-e.patch", Size: 2373 bytes --]

From 03c71cda215fd9183a0fe10cb42394d63e3879c5 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 20 Jul 2018 16:04:01 +0100
Subject: [PATCH] x86/altp2m: Make sure EPTP_INDEX is up-to-date when enabling
 #VE

vmx_vmexit_handler() assumes that if
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set, that the value in
EPTP_INDEX is valid.  Unfortunately, the function which sets this bit
(vmx_vcpu_update_vmfunc_ve) doesn't actually set EPTP_INDEX; it will
only be set the next time vmx_vcpu_update_eptp() is called.

This means that if a vcpu makes a vmexit between these two points, the
EPTP_INDEX it reads will be invalid.  The first time this race happens
for a domain, EPTP_INDEX will most likely be zero, which is the index
for the "host" p2m -- and thus is often correct.  But the second time
this race happens, the value will typically be INVALID_ALTP2M, which
will hit the following BUG:

    BUG_ON(idx >= MAX_ALTP2M);

Worse, if for some reason the current altp2m was *not* `0` during this
window (say, because a toolstack changed the VM to a different view),
then the accounting of active vcpus for an altp2m will be thrown off.

Fix this by always updating EPTP_INDEX to the current altp2m index
when enabling #VE.

Reported-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bcf95f9a5f..bc25daed2c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2191,7 +2191,14 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
             mfn = get_gfn_query_unlocked(d, gfn_x(vcpu_altp2m(v).veinfo_gfn), &t);
 
             if ( !mfn_eq(mfn, INVALID_MFN) )
+            {
                 __vmwrite(VIRT_EXCEPTION_INFO, mfn_x(mfn) << PAGE_SHIFT);
+                /* 
+                 * Make sure we have an up-to-date EPTP_INDEX when
+                 * setting SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS
+                 */
+                __vmwrite(EPTP_INDEX, vcpu_altp2m(v).p2midx);
+            }
             else
                 v->arch.hvm_vmx.secondary_exec_control &=
                     ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
-- 
2.18.0


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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