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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 07/02/2018 08:48 AM, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojocaru@xxxxxxxxxxxxxxx]
>> Sent: Thursday, June 28, 2018 10:35 PM
>>
>> A VM exit handler executed immediately after enabling #VE might
>> find a stale __vmsave()d EPTP_INDEX, stored by calling
>> altp2m_vcpu_destroy() when
>> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS
>> had been enabled by altp2m_vcpu_update_vmfunc_ve().
>>
>> vmx_vmexit_handler() __vmread()s EPTP_INDEX as soon as
>> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set, so if an
>> application enables altp2m on a domain, succesfully calls
>> xc_altp2m_set_vcpu_enable_notify(), then disables altp2m and
>> exits, a second run of said application will likely read the
>> INVALID_ALTP2M EPTP_INDEX set when disabling altp2m in the first
>> run, and crash the host with the BUG_ON(idx >= MAX_ALTP2M),
>> between xc_altp2m_set_vcpu_enable_notify() and
>> xc_altp2m_set_domain_state(..., false).
>>
>> The problem is not restricted to an INVALID_ALTP2M EPTP_INDEX
>> (which can only sanely happen on altp2m uninit), but applies
>> to any stale index previously saved - which means that all
>> altp2m_vcpu_update_vmfunc_ve() calls must also call
>> altp2m_vcpu_update_p2m() after setting
>> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS, in order to make sure
>> that the stored EPTP_INDEX is always valid at
>> vmx_vmexit_handler() time.
>>
>> I don't however fold the two functions into one everywhere,
>> since in p2m_switch_domain_altp2m_by_id() and
>> p2m_switch_vcpu_altp2m_by_id() the extra work done by
>> altp2m_vcpu_update_vmfunc_ve() is unnecessary and has side
>> effects (such as __vmwrite(VM_FUNCTION_CONTROL, ...)).
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>
> 
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>. 
> 
> btw next time please use more descriptive words and less
> function names in commit message.

Fair enough, thanks for the review! Is further action required for this
patch to go in (perhaps George's ack)?


Thanks,
Razvan

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