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 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.  Alternatively, we
could perhaps and xfeature_mask with new_bv which would retain the
safety, but be far less obvious in the failure case on release builds,
most likely resulting in a guest userspace crash.

~Andrew

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