Re: [Xen-devel] [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.07.18 at 09:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 19/07/2018 08:10, Jan Beulich wrote:
>>
>>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const 
>>> struct xsave_hdr *hdr)
>>>  int handle_xsetbv(u32 index, u64 new_bv)
>>>  {
>>>      struct vcpu *curr = current;
>>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>>> +    uint64_t xcr0_max =
>>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>>      u64 mask;
>>>  
>>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>>          return -EOPNOTSUPP;
>>>  
>>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>> +    if ( (new_bv & ~xcr0_max) ||
>>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>          return -EINVAL;
>> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
>> Therefore I'd like to suggest
>>
>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>          return -EINVAL;
>>
>> If you agree, then with the change feel free to add
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Yes - we could make that assertion, but I deliberately opted for the
> code in patch 2 instead.
> 
> If that assertion were to be violated, we'd have a security issue (using
> xstate available in hardware, but unknown to Xen) which would go
> unnoticed, and at the very best, just be a state leak between vcpus.
> 
> I'm open to rearranging the code, but one way or another, the check
> should remain in a release build for robustness.

Well, okay, how about you do as suggested above in this patch, and
then replace / amend the ASSERT() in the next one?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel