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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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