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

Jan



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