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 10:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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?

Layout - perhaps yes. ASSERT_UNREACHABLE() - I don't see a need,
as the domain_crash() ought to be prominent enough.

Jan



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