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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/16 14:47, Jan Beulich wrote:
>>>> On 12.09.16 at 14:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> Well, it's certainly a matter of taste how much of the above to consider
> bounds checking. I for one would take only the middle one as such.

The first is necessary if you want the second, and important as the
calling convention involves the caller modifying xcr0* to match the
incoming state.

The compressed check is admittedly different, but it is currently an ABI
requirement that the data is uncompressed.  This could be lifted if we
alter the ABI.

>
>>>> +    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",
> I think that's the only viable model, considering how the domctl works:
> xcr0_accum needs to represent the combination of features ever
> enabled in XCR0 and XSS.

In which case we should really rename it to xstate_accum then.

>
>> 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.
> How? If the guest never enabled any bit in XSS, how can any such
> bit be set in xstate_bv (which is always a subset of XCR0|XSS).

Currently it cant.  This is a preemptive catch for whomever tries
implementing the first XSS state, and doesn't test migration between
older and newer Xen.

~Andrew

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