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

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

[Razvan Cojocaru]

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@bitdefender.com>

---
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 27.06.18 at 15:12, <rcojocaru@bitdefender.com> wrote:
> 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).

I continue to consider this paragraph unreadable, but perhaps it's
just me. The rest of the description looks reasonable to me now.

Please allow some more time than a single day between versions
though, so others also have a chance to respond.

Jan




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

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

[Razvan Cojocaru]

On 06/27/2018 06:04 PM, Jan Beulich wrote:
>>>> On 27.06.18 at 15:12, <rcojocaru@bitdefender.com> wrote:
>> 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).
> 
> I continue to consider this paragraph unreadable, but perhaps it's
> just me. The rest of the description looks reasonable to me now.

If you (or anyone else) have something specific in mind I'd be happy to
change it to that, otherwise I can also try again (the only concern is
that I might unwantedly continue to be unable to guess correctly at the
desired balance between technical clarity (detail) and general readability).

Is "A VM entry 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 set by altp2m_vcpu_update_vmfunc_ve()." a better first paragraph
perhaps?

> Please allow some more time than a single day between versions
> though, so others also have a chance to respond.

Sorry about that, I misremembered that I had sent V3 yesterday.


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 27.06.18 at 17:25, <rcojocaru@bitdefender.com> wrote:
> On 06/27/2018 06:04 PM, Jan Beulich wrote:
>>>>> On 27.06.18 at 15:12, <rcojocaru@bitdefender.com> wrote:
>>> 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).
>> 
>> I continue to consider this paragraph unreadable, but perhaps it's
>> just me. The rest of the description looks reasonable to me now.
> 
> If you (or anyone else) have something specific in mind I'd be happy to
> change it to that, otherwise I can also try again (the only concern is
> that I might unwantedly continue to be unable to guess correctly at the
> desired balance between technical clarity (detail) and general readability).
> 
> Is "A VM entry 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 set by altp2m_vcpu_update_vmfunc_ve()." a better first paragraph
> perhaps?

To me personally - yes, very much.

Jan



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