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

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

[Razvan Cojocaru]

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 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 V4:
 - The first paragraph has been re-written to be more readable.
 - Fixed a typo in the commit description "cand -> can".
---
 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 V5] x86/altp2m: Fix crash with INVALID_ALTP2M EPTP index

[Jan Beulich]

>>> On 28.06.18 at 16:35, <rcojocaru@bitdefender.com> 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 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

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

[Tian, Kevin]

> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
> Sent: Thursday, June 28, 2018 10:35 PM
> 
> 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 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>
> 

Reviewed-by: Kevin Tian <kevin.tian@intel.com>. 

btw next time please use more descriptive words and less
function names in commit message. 

Thanks
Kevin

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

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

[Razvan Cojocaru]

On 07/02/2018 08:48 AM, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>> Sent: Thursday, June 28, 2018 10:35 PM
>>
>> 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 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>
>>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>. 
> 
> btw next time please use more descriptive words and less
> function names in commit message.

Fair enough, thanks for the review! Is further action required for this
patch to go in (perhaps George's ack)?


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 16.07.18 at 10:30, <rcojocaru@bitdefender.com> wrote:
> On 07/02/2018 08:48 AM, Tian, Kevin wrote:
>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>>> Sent: Thursday, June 28, 2018 10:35 PM
>>>
>>> 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 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>
>>>
>> 
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>. 
>> 
>> btw next time please use more descriptive words and less
>> function names in commit message.
> 
> Fair enough, thanks for the review! Is further action required for this
> patch to go in (perhaps George's ack)?

George's ack, indeed.

Jan



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

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

[George Dunlap]

[-- 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

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

[Razvan Cojocaru]

On 07/20/2018 06:07 PM, George Dunlap wrote:
> 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, thanks for the review, comments and new patch! You're the third
person telling me that the patch description is hard to parse - I'll
definitely work on that skill in the future (and sorry for the
inconvenience).

The vcpu_pause() lead was a red herring in my initial investigation of
the issue, and that is the reason why it didn't make it into the patch
description. The pausing already done is fine.

I've tested your patch on my system (where I can reproduce the crash
with a 100% reproduction rate without it), and I've had no crashes - so
it does seem to have fixed the problem. Thinking about the crash path,
it also makes sense that it would fix the problem - I can't think of any
objections to it.

Let me try the explanation again:

The current situation: when we run twice an altp2m client application
which uses altp2m_vcpu_update_vmfunc_ve() (it _has_ to be twice), the
following happens: after the first run of the application,
altp2m_vcpu_destroy() gets called as part of the cleanup process, and
this stores INVALID_ALTP2M EPTP_INDEX in the VMCS.

altp2m_vcpu_destroy() is what saves INVALID_ALTP2M after the first run
of the client application:

 48 void
 49 altp2m_vcpu_destroy(struct vcpu *v)
 50 {
 51     struct p2m_domain *p2m;
 52
 53     if ( v != current )
 54         vcpu_pause(v);
 55
 56     if ( (p2m = p2m_get_altp2m(v)) )
 57         atomic_dec(&p2m->active_vcpus);
 58
 59     altp2m_vcpu_reset(v);
 60
 61     altp2m_vcpu_update_p2m(v);
 62     altp2m_vcpu_update_vmfunc_ve(v);
 63
 64     if ( v != current )
 65         vcpu_unpause(v);
 66 }

altp2m_vcpu_reset(v) sets av->p2midx = INVALID_ALTP2M, then
altp2m_vcpu_update_p2m(v) saves it.

The _second_ run of the application then calls
altp2m_vcpu_update_vmfunc_ve() again. At this point, EPTP_INDEX ==
INVALID_ALTP2M, and vmx_vcpu_update_vmfunc_ve() only sets
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS (but _not_ EPTP_INDEX also, as
your patch now does). The only function that updates EPTP_INDEX is
vmx_vcpu_update_eptp() - so altp2m_vcpu_update_p2m(v) in my patch.

The pausing is fine, but altp2m_vcpu_update_vmfunc_ve() did not save
EPTP_INDEX.

altp2m_vcpu_update_p2m(v) is called in only 4 places now:
p2m_switch_domain_altp2m_by_id(), p2m_switch_vcpu_altp2m_by_id(),
altp2m_vcpu_initialise() and altp2m_vcpu_destroy().

So at the time of the second run of the application, EPTP_INDEX is still
INVALID_ALTP2M, and vmx_vcpu_update_vmfunc_ve() does nothing about it.

At this point, the first call of vmx_vmexit_handler() will find
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS set and will try to work with the
stored EPTP_INDEX. So you see, the pausing is fine, but the flow is
rather unfortunate.

So basically my patch does what your patch also does in essence. I just
thought that I should change the code that's _not_ VMX-specific in case
altp2m is extended to SVM.

So I hope I've been able to finally clarify things - and if not, it
should be clear to everybody by now that I'm really trying but failing
to be eloquent on this particular topic. :)

Long story short, FWIW:

Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Tested-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

And, of course, many thanks for your help!


Thanks,
Razvan

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

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

[George Dunlap]

On 07/20/2018 05:29 PM, Razvan Cojocaru wrote:
> On 07/20/2018 06:07 PM, George Dunlap wrote:
>> 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, thanks for the review, comments and new patch! You're the third
> person telling me that the patch description is hard to parse - I'll
> definitely work on that skill in the future (and sorry for the
> inconvenience).

No worries -- everything here is a bit of a tangled mess.

> The vcpu_pause() lead was a red herring in my initial investigation of
> the issue, and that is the reason why it didn't make it into the patch
> description. The pausing already done is fine.
> 
> I've tested your patch on my system (where I can reproduce the crash
> with a 100% reproduction rate without it), and I've had no crashes - so
> it does seem to have fixed the problem. Thinking about the crash path,
> it also makes sense that it would fix the problem - I can't think of any
> objections to it.
> 
> Let me try the explanation again:
> 
> The current situation: when we run twice an altp2m client application
> which uses altp2m_vcpu_update_vmfunc_ve() (it _has_ to be twice), the
> following happens: after the first run of the application,
> altp2m_vcpu_destroy() gets called as part of the cleanup process, and
> this stores INVALID_ALTP2M EPTP_INDEX in the VMCS.

Right, I meant, the current situation in terms of the way the code in
Xen / the processor currently behaves / what it expects.

I tried to follow that pattern in my own patch.  The key to the whole
bug is this:

* vmx_vmexit_handler() assumes that is VIRT_EXCEPTION is set, that
EPTP_INDEX is valid

Once you state it that way, you realize, OK that's false.  But why is it
false?

* Because VIRT_EXCEPTION is enabled without touching EPTP_INDEX

That's the core problem.  That description by itself should make anyone
go, "Yeah, that will be a a problem."  The details of how that can go
wrong is just icing on the cake / grep fodder for people looking for how
to fix their own problem.

The reason this ever worked, AFAICT, is that EPTP_INDEX was accidentally
correct.  If we'd initialized EPTP_INDEX with 0xDEADBEEF on VMCS
creation, then you also would have hit the bug.  (In fact, that might
not be a bad idea.)

Furthermore, imagine the following scenario:

* dom0 enables altp2m on domain A
* dom0 switches altp2m to view 1 on domain A
* dom0 enables #VE on domain A
* domain A has a vmexit
  -> At this point, EPTP_INDEX is 0, so the vmexit code will drop a
reference on altp2m index 1 and increase the reference count on altp2m
index 0 #

My patch fixes the above issue, but your patch doesn't (AFAICT).  What
altp2m_vcpu_destroy() did wasn't fundamentally buggy; it just
highlighted the issue by doing the equivalent of putting 0xDEADBEEF in
EPTP_INDEX; and what your patch did was to reverse that, by making
EPTP_INDEX accidentally correct again the next time you ran your test.

(Let me know if I'm wrong about that!)

Stating the problem like that -- saying what the assumption is that's
being violated and why -- is not only more clear, but it also leads to a
more robust solution.

> I just
> thought that I should change the code that's _not_ VMX-specific in case
> altp2m is extended to SVM.

Right, but that assumes the internals are going to be similar somehow.
It's better if you don't have to make assumptions about the internals of
an interface you're calling.

> Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Tested-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Thanks. :-)

 -George

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

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

[Razvan Cojocaru]

On 07/20/2018 08:18 PM, George Dunlap wrote:
> Furthermore, imagine the following scenario:
> 
> * dom0 enables altp2m on domain A
> * dom0 switches altp2m to view 1 on domain A
> * dom0 enables #VE on domain A
> * domain A has a vmexit
>   -> At this point, EPTP_INDEX is 0, so the vmexit code will drop a
> reference on altp2m index 1 and increase the reference count on altp2m
> index 0 #
> 
> My patch fixes the above issue, but your patch doesn't (AFAICT).  What
> altp2m_vcpu_destroy() did wasn't fundamentally buggy; it just
> highlighted the issue by doing the equivalent of putting 0xDEADBEEF in
> EPTP_INDEX; and what your patch did was to reverse that, by making
> EPTP_INDEX accidentally correct again the next time you ran your test.
> 
> (Let me know if I'm wrong about that!)

I do prefer your patch, but unless I'm missing something my patch is
doing the same thing (albeit in a slightly more convoluted manner): it's
calling altp2m_vcpu_update_p2m(v) _inside_
altp2m_vcpu_update_vmfunc_ve(v). That's all it does, other than removing
the (now redundant) explicit altp2m_vcpu_update_p2m(v) call from
altp2m_vcpu_destroy():

https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01898.html

So for every hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v) (i.e. the vmx.c
function) that gets called, I also call altp2m_vcpu_update_p2m(v), which
properly sets EPTP_INDEX (just as your patch does by __vmwrite()ing it
directly in vmx_vcpu_update_vmfunc_ve(), but in a roundabout manner).

Did I misunderstand something?


Thanks,
Razvan

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

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

[George Dunlap]

On 07/20/2018 07:02 PM, Razvan Cojocaru wrote:
> On 07/20/2018 08:18 PM, George Dunlap wrote:
>> Furthermore, imagine the following scenario:
>>
>> * dom0 enables altp2m on domain A
>> * dom0 switches altp2m to view 1 on domain A
>> * dom0 enables #VE on domain A
>> * domain A has a vmexit
>>   -> At this point, EPTP_INDEX is 0, so the vmexit code will drop a
>> reference on altp2m index 1 and increase the reference count on altp2m
>> index 0 #
>>
>> My patch fixes the above issue, but your patch doesn't (AFAICT).  What
>> altp2m_vcpu_destroy() did wasn't fundamentally buggy; it just
>> highlighted the issue by doing the equivalent of putting 0xDEADBEEF in
>> EPTP_INDEX; and what your patch did was to reverse that, by making
>> EPTP_INDEX accidentally correct again the next time you ran your test.
>>
>> (Let me know if I'm wrong about that!)
> 
> I do prefer your patch, but unless I'm missing something my patch is
> doing the same thing (albeit in a slightly more convoluted manner): it's
> calling altp2m_vcpu_update_p2m(v) _inside_
> altp2m_vcpu_update_vmfunc_ve(v). That's all it does, other than removing
> the (now redundant) explicit altp2m_vcpu_update_p2m(v) call from
> altp2m_vcpu_destroy():
> 
> https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01898.html
> 
> So for every hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v) (i.e. the vmx.c
> function) that gets called, I also call altp2m_vcpu_update_p2m(v), which
> properly sets EPTP_INDEX (just as your patch does by __vmwrite()ing it
> directly in vmx_vcpu_update_vmfunc_ve(), but in a roundabout manner).
> 
> Did I misunderstand something?

No, you didn't -- sorry, I must have been quite tired at that point. :-)

What I was actually thinking of was that in your patch, the update
happens in different vmcs_enter/exit critical section, whereas in mine
it's in the same section.

Looking through the code, it seems that the vmcs_enter/exit acts as a
lock, by pausing and unpausing the vcpu if it's not the one we're
currently running on (as well as actually grabbing a lock to prevent
concurrent modification).  altp2m_vcpu_destroy() calls
altp2m_vcpu_update_vmfunc_ve() with the vcpu paused, but the
HVMOP_altp2m_vcpu_enable_notify hypercall doesn't seem to; which (I
think) means there could still be a point between
vmx_vcpu_update_vmfunc_ve() and vmx_vcpu_update_eptp() where a vcpu
could run and get the wrong EPTP_INDEX.

It's possible my analysis is wrong there (I'm not intimately familiar
with the code), but I think my patch is better anyway for a couple of
reasons:

* The logic to keep EPTP_INDEX in sync is explicit, and all in the same
file.

* It doesn't do unnecessary updates to other bits of state

* If we ever have reason to call vmx_vcpu_update_vmfunc_ve() directly,
we won't re-introduce this bug.  (Or to put it a different way: We don't
have to remember that we can't call it directly.)

Now we just need to get the VMX maintainers to sign off on it. :-)  Jun
/ Kevin, any thoughts?

 -George

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

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

[Razvan Cojocaru]

On 07/23/2018 01:29 PM, George Dunlap wrote:
> It's possible my analysis is wrong there (I'm not intimately familiar
> with the code), but I think my patch is better anyway for a couple of
> reasons:
> 
> * The logic to keep EPTP_INDEX in sync is explicit, and all in the same
> file.
> 
> * It doesn't do unnecessary updates to other bits of state
> 
> * If we ever have reason to call vmx_vcpu_update_vmfunc_ve() directly,
> we won't re-introduce this bug.  (Or to put it a different way: We don't
> have to remember that we can't call it directly.)

Very good reasons, and I completely agree. Thanks again for your help!


Thanks,
Razvan

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

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

[Razvan Cojocaru]

On 07/23/2018 01:29 PM, George Dunlap wrote:
> On 07/20/2018 07:02 PM, Razvan Cojocaru wrote:
>> On 07/20/2018 08:18 PM, George Dunlap wrote:
>>> Furthermore, imagine the following scenario:
>>>
>>> * dom0 enables altp2m on domain A
>>> * dom0 switches altp2m to view 1 on domain A
>>> * dom0 enables #VE on domain A
>>> * domain A has a vmexit
>>>   -> At this point, EPTP_INDEX is 0, so the vmexit code will drop a
>>> reference on altp2m index 1 and increase the reference count on altp2m
>>> index 0 #
>>>
>>> My patch fixes the above issue, but your patch doesn't (AFAICT).  What
>>> altp2m_vcpu_destroy() did wasn't fundamentally buggy; it just
>>> highlighted the issue by doing the equivalent of putting 0xDEADBEEF in
>>> EPTP_INDEX; and what your patch did was to reverse that, by making
>>> EPTP_INDEX accidentally correct again the next time you ran your test.
>>>
>>> (Let me know if I'm wrong about that!)
>>
>> I do prefer your patch, but unless I'm missing something my patch is
>> doing the same thing (albeit in a slightly more convoluted manner): it's
>> calling altp2m_vcpu_update_p2m(v) _inside_
>> altp2m_vcpu_update_vmfunc_ve(v). That's all it does, other than removing
>> the (now redundant) explicit altp2m_vcpu_update_p2m(v) call from
>> altp2m_vcpu_destroy():
>>
>> https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01898.html
>>
>> So for every hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v) (i.e. the vmx.c
>> function) that gets called, I also call altp2m_vcpu_update_p2m(v), which
>> properly sets EPTP_INDEX (just as your patch does by __vmwrite()ing it
>> directly in vmx_vcpu_update_vmfunc_ve(), but in a roundabout manner).
>>
>> Did I misunderstand something?
> 
> No, you didn't -- sorry, I must have been quite tired at that point. :-)
> 
> What I was actually thinking of was that in your patch, the update
> happens in different vmcs_enter/exit critical section, whereas in mine
> it's in the same section.
> 
> Looking through the code, it seems that the vmcs_enter/exit acts as a
> lock, by pausing and unpausing the vcpu if it's not the one we're
> currently running on (as well as actually grabbing a lock to prevent
> concurrent modification).  altp2m_vcpu_destroy() calls
> altp2m_vcpu_update_vmfunc_ve() with the vcpu paused, but the
> HVMOP_altp2m_vcpu_enable_notify hypercall doesn't seem to; which (I
> think) means there could still be a point between
> vmx_vcpu_update_vmfunc_ve() and vmx_vcpu_update_eptp() where a vcpu
> could run and get the wrong EPTP_INDEX.
> 
> It's possible my analysis is wrong there (I'm not intimately familiar
> with the code), but I think my patch is better anyway for a couple of
> reasons:
> 
> * The logic to keep EPTP_INDEX in sync is explicit, and all in the same
> file.
> 
> * It doesn't do unnecessary updates to other bits of state
> 
> * If we ever have reason to call vmx_vcpu_update_vmfunc_ve() directly,
> we won't re-introduce this bug.  (Or to put it a different way: We don't
> have to remember that we can't call it directly.)
> 
> Now we just need to get the VMX maintainers to sign off on it. :-)  Jun
> / Kevin, any thoughts?

Ping for the VMX maintainers?


Thanks,
Razvan

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

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

[Tian, Kevin]

> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
> Sent: Wednesday, August 1, 2018 5:02 PM
> 
> On 07/23/2018 01:29 PM, George Dunlap wrote:
> > On 07/20/2018 07:02 PM, Razvan Cojocaru wrote:
> >> On 07/20/2018 08:18 PM, George Dunlap wrote:
> >>> Furthermore, imagine the following scenario:
> >>>
> >>> * dom0 enables altp2m on domain A
> >>> * dom0 switches altp2m to view 1 on domain A
> >>> * dom0 enables #VE on domain A
> >>> * domain A has a vmexit
> >>>   -> At this point, EPTP_INDEX is 0, so the vmexit code will drop a
> >>> reference on altp2m index 1 and increase the reference count on
> altp2m
> >>> index 0 #
> >>>
> >>> My patch fixes the above issue, but your patch doesn't (AFAICT).  What
> >>> altp2m_vcpu_destroy() did wasn't fundamentally buggy; it just
> >>> highlighted the issue by doing the equivalent of putting 0xDEADBEEF in
> >>> EPTP_INDEX; and what your patch did was to reverse that, by making
> >>> EPTP_INDEX accidentally correct again the next time you ran your test.
> >>>
> >>> (Let me know if I'm wrong about that!)
> >>
> >> I do prefer your patch, but unless I'm missing something my patch is
> >> doing the same thing (albeit in a slightly more convoluted manner): it's
> >> calling altp2m_vcpu_update_p2m(v) _inside_
> >> altp2m_vcpu_update_vmfunc_ve(v). That's all it does, other than
> removing
> >> the (now redundant) explicit altp2m_vcpu_update_p2m(v) call from
> >> altp2m_vcpu_destroy():
> >>
> >> https://lists.xenproject.org/archives/html/xen-devel/2018-
> 06/msg01898.html
> >>
> >> So for every hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v) (i.e. the
> vmx.c
> >> function) that gets called, I also call altp2m_vcpu_update_p2m(v), which
> >> properly sets EPTP_INDEX (just as your patch does by __vmwrite()ing it
> >> directly in vmx_vcpu_update_vmfunc_ve(), but in a roundabout
> manner).
> >>
> >> Did I misunderstand something?
> >
> > No, you didn't -- sorry, I must have been quite tired at that point. :-)
> >
> > What I was actually thinking of was that in your patch, the update
> > happens in different vmcs_enter/exit critical section, whereas in mine
> > it's in the same section.
> >
> > Looking through the code, it seems that the vmcs_enter/exit acts as a
> > lock, by pausing and unpausing the vcpu if it's not the one we're
> > currently running on (as well as actually grabbing a lock to prevent
> > concurrent modification).  altp2m_vcpu_destroy() calls
> > altp2m_vcpu_update_vmfunc_ve() with the vcpu paused, but the
> > HVMOP_altp2m_vcpu_enable_notify hypercall doesn't seem to; which (I
> > think) means there could still be a point between
> > vmx_vcpu_update_vmfunc_ve() and vmx_vcpu_update_eptp() where a
> vcpu
> > could run and get the wrong EPTP_INDEX.
> >
> > It's possible my analysis is wrong there (I'm not intimately familiar
> > with the code), but I think my patch is better anyway for a couple of
> > reasons:
> >
> > * The logic to keep EPTP_INDEX in sync is explicit, and all in the same
> > file.
> >
> > * It doesn't do unnecessary updates to other bits of state
> >
> > * If we ever have reason to call vmx_vcpu_update_vmfunc_ve() directly,
> > we won't re-introduce this bug.  (Or to put it a different way: We don't
> > have to remember that we can't call it directly.)
> >
> > Now we just need to get the VMX maintainers to sign off on it. :-)  Jun
> > / Kevin, any thoughts?
> 
> Ping for the VMX maintainers?
> 

yes, it makes sense to me.

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

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

[Razvan Cojocaru]

On 08/02/2018 09:32 AM, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>> Sent: Wednesday, August 1, 2018 5:02 PM
>>
>> On 07/23/2018 01:29 PM, George Dunlap wrote:
>>> On 07/20/2018 07:02 PM, Razvan Cojocaru wrote:
>>>> On 07/20/2018 08:18 PM, George Dunlap wrote:
>>>>> Furthermore, imagine the following scenario:
>>>>>
>>>>> * dom0 enables altp2m on domain A
>>>>> * dom0 switches altp2m to view 1 on domain A
>>>>> * dom0 enables #VE on domain A
>>>>> * domain A has a vmexit
>>>>>   -> At this point, EPTP_INDEX is 0, so the vmexit code will drop a
>>>>> reference on altp2m index 1 and increase the reference count on
>> altp2m
>>>>> index 0 #
>>>>>
>>>>> My patch fixes the above issue, but your patch doesn't (AFAICT).  What
>>>>> altp2m_vcpu_destroy() did wasn't fundamentally buggy; it just
>>>>> highlighted the issue by doing the equivalent of putting 0xDEADBEEF in
>>>>> EPTP_INDEX; and what your patch did was to reverse that, by making
>>>>> EPTP_INDEX accidentally correct again the next time you ran your test.
>>>>>
>>>>> (Let me know if I'm wrong about that!)
>>>>
>>>> I do prefer your patch, but unless I'm missing something my patch is
>>>> doing the same thing (albeit in a slightly more convoluted manner): it's
>>>> calling altp2m_vcpu_update_p2m(v) _inside_
>>>> altp2m_vcpu_update_vmfunc_ve(v). That's all it does, other than
>> removing
>>>> the (now redundant) explicit altp2m_vcpu_update_p2m(v) call from
>>>> altp2m_vcpu_destroy():
>>>>
>>>> https://lists.xenproject.org/archives/html/xen-devel/2018-
>> 06/msg01898.html
>>>>
>>>> So for every hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v) (i.e. the
>> vmx.c
>>>> function) that gets called, I also call altp2m_vcpu_update_p2m(v), which
>>>> properly sets EPTP_INDEX (just as your patch does by __vmwrite()ing it
>>>> directly in vmx_vcpu_update_vmfunc_ve(), but in a roundabout
>> manner).
>>>>
>>>> Did I misunderstand something?
>>>
>>> No, you didn't -- sorry, I must have been quite tired at that point. :-)
>>>
>>> What I was actually thinking of was that in your patch, the update
>>> happens in different vmcs_enter/exit critical section, whereas in mine
>>> it's in the same section.
>>>
>>> Looking through the code, it seems that the vmcs_enter/exit acts as a
>>> lock, by pausing and unpausing the vcpu if it's not the one we're
>>> currently running on (as well as actually grabbing a lock to prevent
>>> concurrent modification).  altp2m_vcpu_destroy() calls
>>> altp2m_vcpu_update_vmfunc_ve() with the vcpu paused, but the
>>> HVMOP_altp2m_vcpu_enable_notify hypercall doesn't seem to; which (I
>>> think) means there could still be a point between
>>> vmx_vcpu_update_vmfunc_ve() and vmx_vcpu_update_eptp() where a
>> vcpu
>>> could run and get the wrong EPTP_INDEX.
>>>
>>> It's possible my analysis is wrong there (I'm not intimately familiar
>>> with the code), but I think my patch is better anyway for a couple of
>>> reasons:
>>>
>>> * The logic to keep EPTP_INDEX in sync is explicit, and all in the same
>>> file.
>>>
>>> * It doesn't do unnecessary updates to other bits of state
>>>
>>> * If we ever have reason to call vmx_vcpu_update_vmfunc_ve() directly,
>>> we won't re-introduce this bug.  (Or to put it a different way: We don't
>>> have to remember that we can't call it directly.)
>>>
>>> Now we just need to get the VMX maintainers to sign off on it. :-)  Jun
>>> / Kevin, any thoughts?
>>
>> Ping for the VMX maintainers?
>>
> 
> yes, it makes sense to me.

Thanks!

I've just re-sent the patch as " [PATCH V6] x86/altp2m: Make sure
EPTP_INDEX is up-to-date when enabling #VE" on George's suggestion, so
that it will appear on the list in the traditional process as well.


Thanks,
Razvan

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