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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>  void compress_xsave_states(struct vcpu *v, const void *src, unsigned int 
> size)
>  {
>      struct xsave_struct *xsave = v->arch.xsave_area;
>      uint16_t comp_offsets[sizeof(xfeature_mask)*8];
> -    u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
> -    u64 valid;
> +    u64 xstate_bv, valid;
> +
> +    BUG_ON(!v->arch.xcr0_accum);
> +    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
> +    BUG_ON(xsave_area_compressed(src));
>  
> -    ASSERT(!xsave_area_compressed(src));

Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT().

> +    xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
>  
>      if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
>      {
> +        /*
> +         * TODO: This logic doesn't currently handle restoration of xsave
> +         * state which would force the vcpu from uncompressed to compressed.
> +         */
> +        BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY);

I don't think this is a valid concern of yours: The function can't be
used to restore features not xcr0_accum anyway (or else that
field would need updating). Afaict validate_xstate() already prevents
this as intended.

> @@ -262,11 +281,13 @@ void compress_xsave_states(struct vcpu *v, const void 
> *src, unsigned int size)
>          unsigned int index = fls(feature) - 1;
>          void *dest = get_xsave_addr(xsave, comp_offsets, index);
>  
> -        if ( dest )
> -        {
> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> -            memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]);
> -        }
> +        /*
> +         * We previously verified xstate_bv.  If we don't have valid
> +         * comp_offset[] information, something is very broken.
> +         */
> +        BUG_ON(!dest);

Are you sure? In compressed format bits in xstate_bv may be clear
while xcomp_bv has them set. In such a case there's nowhere to
copy to, and hence dest is NULL when coming here.

Jan



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