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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.18 at 12:55, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 06/26/2018 01:26 PM, Jan Beulich wrote:
>>>>> On 25.06.18 at 16:10, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/mm/altp2m.c
>>> +++ b/xen/arch/x86/mm/altp2m.c
>>> @@ -58,8 +58,8 @@ altp2m_vcpu_destroy(struct vcpu *v)
>>>  
>>>      altp2m_vcpu_reset(v);
>>>  
>>> -    altp2m_vcpu_update_p2m(v);
>>>      altp2m_vcpu_update_vmfunc_ve(v);
>>> +    altp2m_vcpu_update_p2m(v);
>> 
>> I agree this addresses this specific incarnation of the problem. However,
>> if the vCPU indeed runs while being manipulated, I don't think you get
>> rid of the race this way. For one, there is e.g. a solitary call to
>> altp2m_vcpu_update_vmfunc_ve() in the handling of
>> HVMOP_altp2m_vcpu_enable_notify. That'll lead to
>> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS getting set, but
>> EPTP_INDEX won't be written. Whatever previous value is in place
>> would then be read back in VM exit handling.
>> 
>> With that it also looks to me as if the two step (and hence non-atomic
>> from the perspective of the guest) update is a problem. Even with the
>> change above, the VM exit may now happen exactly between the two
>> function calls.
>> 
>> It seems to me that pausing the vCPU is almost unavoidable (and then
>> the ordering of the two calls is relevant only because
>> vmx_vcpu_update_eptp() would better respect the intended new
>> setting of SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS).
> 
> I see. Would not then your scenario be covered by
> altp2m_vcpu_update_vmfunc_ve() always calling altp2m_vcpu_update_p2m()
> at the very end (and removing duplicate calls to altp2m_vcpu_update_p2m())?

No - there would then still be a window of time where the bit is set but
the index was not (yet) written.

> Unless something is very wrong, all calls to
> altp2m_vcpu_update_vmfunc_ve() _should_ happen within a pause, so no
> calls to the VM exit handler should occur between them if they become a
> single block of logic within altp2m_vcpu_update_vmfunc_ve().

Did I misunderstand your analysis mails then? It looked to me as if
you were observing exactly such races, because of the vCPU not
being paused.

>> I also think that you'd better Cc the VMX maintainers here, even if
>> the fix itself is outside the code their maintainership covers.
> 
> Sorry, I don't follow. The MAINTAINTERS file lists only Jun Nakajima and
> Kevin Tian under "INTEL(R) VT FOR X86 (VT-X)" (where
> xen/arch/x86/hvm/vmx/ and xen/include/asm-x86/hvm/vmx/ live), and they
> are both CCd here. What am I missing?

Your patch submission didn't have them on Cc, I've added them when
replying.

Jan



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