Re: [Xen-devel] [PATCH] x86/hvm: Set the emulation context correctly in hvmemul_cmpxchg()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.16 at 15:19, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1031,13 +1031,17 @@ static int hvmemul_cmpxchg(
>  
>      if ( unlikely(hvmemul_ctxt->set_context) )
>      {
> -        int rc = set_context_data(p_new, bytes);
> +        int rc = set_context_data(p_old, bytes);

So this addresses one half of the problem mentioned, but not the
other: You're still (unlike in all other cases where set_context_data()
is being used) modifying data owned by the caller, which may end
in it getting confused. I admit the semantics of the ->cmpxchg()
callback aren't well defined, but current behavior is clearly to leave
both buffers untouched even if at least p_new can't be constified.
And if p_old was meant to get modified in any way, it clearly could
only be to return back the old value an actual CMPXCHG may have
found in memory (which afaict could be different from whatever
override the introspection app may have enforced).

>          if ( rc != X86EMUL_OKAY )
>              return rc;
>      }
>  
> -    /* Fix this in case the guest is really relying on r-m-w atomicity. */
> +    /*
> +     * Fix this in case the guest is really relying on r-m-w atomicity.
> +     * Please note that while the set_context code is provided here for
> +     * consistency, p_old is unused.
> +     */
>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>  }

So with this I wonder btw. why your patch (mostly) fixing this
shortcoming (while adding proper LOCK handling) never made it
to a version that could be committed.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel