Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -176,6 +187,11 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>      u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
>      u64 valid;
>  
> +    /* Check there is state to serialise (i.e. at least an XSAVE_HDR) */
> +    BUG_ON(!v->arch.xcr0_accum);
> +    /* Check there is the correct room to decompress into. */
> +    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));

Further down I see you convert an ASSERT() to BUG_ON(), but I
wonder why you do that and why the two above can't be ASSERT()
too. xstate_ctxt_size() is not always cheap.

> @@ -189,6 +205,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>       * Copy legacy XSAVE area and XSAVE hdr area.
>       */
>      memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE);
> +    memset(dest + XSTATE_AREA_MIN_SIZE, 0, size - XSTATE_AREA_MIN_SIZE);
>  
>      ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
>  
> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>  
>          if ( src )
>          {
> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);

Surely converting an ASSERT() to BUG_ON() means inverting the
relational operator used?

>              memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
>          }
> -        else
> -            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);

So I have difficulty seeing why this memset() wasn't sufficient: It
precisely covers for the respective component being in default
state. Or wait - this was fine if intermediate bits were clear in
xstate_bv, but not if clear-but-valid ones weren't followed by
another set one. Nor would gaps between components have been
taken care of. I think the commit message could be made more
explicit in this regard (of course unless I'm overlooking yet another
aspect).

Jan


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