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

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

[Razvan Cojocaru]

When SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set,
vmx_vcpu_update_eptp() __vmwrites() EPTP_INDEX in
altp2m_vcpu_destroy(). This means that when disabling altp2m on a
domain after xc_altp2m_set_vcpu_enable_notify() has been
successfully called, EPTP_INDEX ends up being stored as
INVALID_ALTP2M. This makes it possible for vmx_vmexit_handler()
to __vmread() the stale value after a subsequent call to
xc_altp2m_set_vcpu_enable_notify(), and BUG_ON(idx >= MAX_ALTP2M).

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V2:
 - Changed verb tense in commit message subject.
 - Now calling altp2m_vcpu_update_p2m() as part of
   altp2m_vcpu_update_vmfunc_ve() (and removed the explicit
   altp2m_vcpu_update_p2m() call from altp2m_vcpu_destroy()).
---
 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 V3] x86/altp2m: Fix crash with INVALID_ALTP2M EPTP index

[Jan Beulich]

>>> On 26.06.18 at 16:21, <rcojocaru@bitdefender.com> wrote:
> When SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set,
> vmx_vcpu_update_eptp() __vmwrites() EPTP_INDEX in
> altp2m_vcpu_destroy(). This means that when disabling altp2m on a
> domain after xc_altp2m_set_vcpu_enable_notify() has been
> successfully called, EPTP_INDEX ends up being stored as
> INVALID_ALTP2M. This makes it possible for vmx_vmexit_handler()
> to __vmread() the stale value after a subsequent call to
> xc_altp2m_set_vcpu_enable_notify(), and BUG_ON(idx >= MAX_ALTP2M).

I'm fine with the code change now, but I think this 3rd approach
of addressing the issue needs the description to be changed.
Already on v2 it wouldn't have become clear to me what the
issue was from just reading the description. In particular you now
want to point out why the change is correct / necessary also for
the other invocation of altp2m_vcpu_update_vmfunc_ve(). It
would also be helpful to have a statement on why other
altp2m_vcpu_update_p2m() invocations don't need to be
prefixed (now: replaced) by altp2m_vcpu_update_vmfunc_ve().
In the end it might well be that folding the two hooks into one is
the best course of action.

Jan



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

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

[Razvan Cojocaru]

On 06/27/2018 12:46 PM, Jan Beulich wrote:
>>>> On 26.06.18 at 16:21, <rcojocaru@bitdefender.com> wrote:
>> When SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set,
>> vmx_vcpu_update_eptp() __vmwrites() EPTP_INDEX in
>> altp2m_vcpu_destroy(). This means that when disabling altp2m on a
>> domain after xc_altp2m_set_vcpu_enable_notify() has been
>> successfully called, EPTP_INDEX ends up being stored as
>> INVALID_ALTP2M. This makes it possible for vmx_vmexit_handler()
>> to __vmread() the stale value after a subsequent call to
>> xc_altp2m_set_vcpu_enable_notify(), and BUG_ON(idx >= MAX_ALTP2M).
> 
> I'm fine with the code change now, but I think this 3rd approach
> of addressing the issue needs the description to be changed.
> Already on v2 it wouldn't have become clear to me what the
> issue was from just reading the description. In particular you now
> want to point out why the change is correct / necessary also for
> the other invocation of altp2m_vcpu_update_vmfunc_ve(). It
> would also be helpful to have a statement on why other
> altp2m_vcpu_update_p2m() invocations don't need to be
> prefixed (now: replaced) by altp2m_vcpu_update_vmfunc_ve().
> In the end it might well be that folding the two hooks into one is
> the best course of action.

I'll do my best to make the description more readable. As for folding
the two hooks into one (I assume you mean having a single function, such
as, e.g. altp2m_vcpu_update_ve_and_p2m() and removing the other two), it
looks like vmx_vcpu_update_vmfunc_ve() does a few things that would be
unnnecessary (not optimal) in the general case. For example it calls
__vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);, which
shouldn't necessarily happen at the callsites of
altp2m_vcpu_update_p2m(v) in p2m.c (in p2m_switch_vcpu_altp2m_by_id()
and p2m_switch_domain_altp2m_by_id()). So from that point of view, it
may be worth to keep both altp2m_vcpu_update_p2m() and
altp2m_vcpu_update_vmfunc_ve() (the latter still always needing to call
the former to do its job properly).

It's possible that I've misunderstood your comment here though.


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 27.06.18 at 12:18, <rcojocaru@bitdefender.com> wrote:
> On 06/27/2018 12:46 PM, Jan Beulich wrote:
>>>>> On 26.06.18 at 16:21, <rcojocaru@bitdefender.com> wrote:
>>> When SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set,
>>> vmx_vcpu_update_eptp() __vmwrites() EPTP_INDEX in
>>> altp2m_vcpu_destroy(). This means that when disabling altp2m on a
>>> domain after xc_altp2m_set_vcpu_enable_notify() has been
>>> successfully called, EPTP_INDEX ends up being stored as
>>> INVALID_ALTP2M. This makes it possible for vmx_vmexit_handler()
>>> to __vmread() the stale value after a subsequent call to
>>> xc_altp2m_set_vcpu_enable_notify(), and BUG_ON(idx >= MAX_ALTP2M).
>> 
>> I'm fine with the code change now, but I think this 3rd approach
>> of addressing the issue needs the description to be changed.
>> Already on v2 it wouldn't have become clear to me what the
>> issue was from just reading the description. In particular you now
>> want to point out why the change is correct / necessary also for
>> the other invocation of altp2m_vcpu_update_vmfunc_ve(). It
>> would also be helpful to have a statement on why other
>> altp2m_vcpu_update_p2m() invocations don't need to be
>> prefixed (now: replaced) by altp2m_vcpu_update_vmfunc_ve().
>> In the end it might well be that folding the two hooks into one is
>> the best course of action.
> 
> I'll do my best to make the description more readable. As for folding
> the two hooks into one (I assume you mean having a single function, such
> as, e.g. altp2m_vcpu_update_ve_and_p2m() and removing the other two), it
> looks like vmx_vcpu_update_vmfunc_ve() does a few things that would be
> unnnecessary (not optimal) in the general case. For example it calls
> __vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);, which
> shouldn't necessarily happen at the callsites of
> altp2m_vcpu_update_p2m(v) in p2m.c (in p2m_switch_vcpu_altp2m_by_id()
> and p2m_switch_domain_altp2m_by_id()). So from that point of view, it
> may be worth to keep both altp2m_vcpu_update_p2m() and
> altp2m_vcpu_update_vmfunc_ve() (the latter still always needing to call
> the former to do its job properly).
> 
> It's possible that I've misunderstood your comment here though.

I think you've understood me right; what you say makes sense at the
first glance. Please summarize this in the commit message, so that
further questions (perhaps also by others) can be avoided.

Jan



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