[Xen-devel] [PATCH V4] x86/altp2m: Fix crash with INVALID_ALTP2M EPTP index

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

xc_altp2m_set_vcpu_enable_notify() ends up calling
altp2m_vcpu_update_vmfunc_ve(), which sets the
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS bit on
vmx_secondary_exec_control. A subsequent call to
xc_altp2m_set_domain_state(..., false) (i.e. disabling altp2m
for the domain) ends up calling altp2m_vcpu_destroy(), which
calls (in this order) altp2m_vcpu_reset() (which sets the
current EPTP index to INVALID_ALTP2M), altp2m_vcpu_update_p2m()
(which __vmwrite()s EPTP_INDEX as INVALID_ALTP2M if
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set), and
altp2m_vcpu_update_vmfunc_ve() (which finally clears
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS).

However, 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 cand 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 don't however fold the two functions into one everywhere,
since in p2m_switch_domain_altp2m_by_id() and
p2m_switch_vcpu_altp2m_by_id() the extra work done by
altp2m_vcpu_update_vmfunc_ve() is unnecessary and has side
effects (such as __vmwrite(VM_FUNCTION_CONTROL, ...)).

Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>

---
Changes since V3:
 - Expanded and clarified the patch commit message.
---
 xen/arch/x86/mm/altp2m.c      | 1 -
 xen/include/asm-x86/hvm/hvm.h | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 930bdc2..9d60dc4 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -58,7 +58,6 @@ altp2m_vcpu_destroy(struct vcpu *v)
 
     altp2m_vcpu_reset(v);
 
-    altp2m_vcpu_update_p2m(v);
     altp2m_vcpu_update_vmfunc_ve(v);
 
     if ( v != current )
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index ef5e198..0bf6913 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -630,6 +630,8 @@ static inline void altp2m_vcpu_update_vmfunc_ve(struct vcpu 
*v)
 {
     if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
         hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
+
+    altp2m_vcpu_update_p2m(v);
 }
 
 /* emulates #VE */
-- 
2.7.4


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