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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/16 12:41, Jan Beulich wrote:
>>>> 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.

This isn't a fastpath, and the cpuid work will make xstate_ctxt_size()
into an O(1) operation.

Furthermore, following the investigation of XSA-186, I will not use
assertions for bounds checking.  The potential damage of omitting the
check far outweighs the overhead of the unconditional check.

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

Very true.  It is unfortunate that all of this is dead code, and
impossible to test.  I also had half a mind to explicitly #if 0 it out
to leave people in no illusion that it ever might have been tested.

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

No it doesn't.  The loop skips over all bits which are not set in xstate_bv.

I had (erroneously) come to the conclusion that the "if ( src )" check
only caught the case where we had bad comp_offsets[] information, but
rereading the logic, that case would actually corrupt the legacy SSE header.

Overall, it turns out that the "if ( src )" is unconditionally taken.

~Andrew

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