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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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