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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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