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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/07/2018 09:34, Jan Beulich wrote:
>>>> On 19.07.18 at 10:26, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 19/07/2018 09:21, Jan Beulich wrote:
>>>>>> 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?
>> Why?  From a bisection point of view that's strictly worse that the
>> order of changes presented here, even if it is a condition we don't
>> expect to hit, and its unnecessary work as the end result is still going
>> to remain the same.
> The end result is the same, yes, so it doesn't matter all that much.
> But
>
>     if ( (new_bv & ~xcr0_max) ||
>          (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>
> is still not making obvious that there's actually redundancy there
> here, while
>
>     ASSERT(!(xcr0_max & ~xfeature_mask));
>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>
> does. So yes, it's minor enough that you may feel free to ignore
> my comments and put in the patches as they are.

I can tweak patch 2 to follow this layout rather than the current, and
can leave some comments and an ASSERT_UNREACHABLE() after the printk()
if you'd like?

~Andrew

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