On 06/28/2018 03:35 PM, Razvan Cojocaru wrote:
> 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'm sorry, this description still doesn't make hardly any sense to me,
nor the solution, even after reading all the previous threads on the
issue.  The description doesn't, for instance, mention vcpu_pause() at
all, in spite of the fact that it seems (from the previous discussion)
that this is a critical part of why this solution works; nor is there
any comment in the code about the required discipline regarding
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS,  making it fairly likely that
someone will re-introduce a bug like this in the future.

My normal template for something like this is
1. Explain what the current situation is
2. Explain why that's a problem
3. Describe what you're changing and how it fixes it.

I can't help but think the right thing to do here is in vmx.c somewhere
-- it is, after all, code in vmx.c that:
1. Sets and clears SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS
2. Writes EPTP_INDEX
3. Assumes that SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS => EPTP_INDEX is
valid.

What about something like the attached, instead (compile-tested only)?

 -George