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

["Tian, Kevin" <kevin.tian@intel.com>]

> 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