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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 06/26/2018 01:26 PM, Jan Beulich wrote:
>>>> On 25.06.18 at 16:10, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> When SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set,
>> vmx_vcpu_update_eptp() __vmwrites() EPTP_INDEX in
>> altp2m_vcpu_destroy(). This means that when disabling altp2m on a
>> domain after xc_altp2m_set_vcpu_enable_notify() has been
>> successfully called, EPTP_INDEX ends up being stored as
>> INVALID_ALTP2M. This makes it possible for vmx_vmexit_handler()
>> to __vmread() the stale value after a subsequent call to
>> xc_altp2m_set_vcpu_enable_notify(), and BUG_ON(idx >= MAX_ALTP2M).
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>
>> ---
>> Changes since V1:
>>  - Re-wrote the fix to affect the altp2m code instead of the code
>>    around the BUG_ON().
>>  - Updated the patch description (and title - since the crash
>>    is really a host, not a domain, crash).
> 
> I think we've been there before: Why "fixed" rather than "fix" (or
> "avoid") in the title? My general view is that a title says what a
> patch does, not what the state is after it has been applied.

I'll change the patch subject.

>> --- 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())?

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().

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


Thanks,
Razvan

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