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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 17:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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:
>>>>> +    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.

Right.

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

I still don't see why you say "currently" when this is an architectural
(of how Xen behaves, not hardware architecture) thing: xcr0_accum,
as its name says, only ever gets bits added to it, and if some bit is
clear there it can't possibly be set in xstate_bv or xcomp_bv (for
the latter of course with the exception of the compressed bit). So by
having made it past 

     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {

the BUG_ON() you add can't possibly trigger (or else we have much
more severe problems). In particular this has nothing to do with
whether any XSS states are being used.

Jan


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