[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>>> On 25.06.18 at 16:10, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 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
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

I also think that you'd better Cc the VMX maintainers here, even if
the fix itself is outside the code their maintainership covers.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.