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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> --- 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 also think that you'd better Cc the VMX maintainers here, even if
the fix itself is outside the code their maintainership covers.

Jan



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