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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/16 13:27, Jan Beulich wrote:
>>>> 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().

Same answer.

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

This is all currently dead code.  I guess the question really depends on
what we plan to do with compressed states.

Strictly speaking, no XSAVES state can every be present in xcr0, by
design.  If we retroactively consider xcr0_accum to be "all states in
use", then the if condition in context does become relevant when Xen
starts supporting XSAVES-only components.

In such a case, it is definitely wrong to memcpy() the uncompressed
buffer, as Xen will try and use xrstors and corrupt all guest state.

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

This logic was based on my wrong analysis.  However, this loop only
executes for bits set in xstate_bv, so "if ( dest )" will never be false.

~Andrew

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