Re: [PATCH v5 14/28] x86/fpu/xstate: Prevent unauthorised use of dynamic user state

["Bae, Chang Seok" <chang.seok.bae@intel.com>]

On Jun 29, 2021, at 11:50, Hansen, Dave <dave.hansen@intel.com> wrote:
> On 6/29/21 11:35 AM, Bae, Chang Seok wrote:
>>        if (likely(use_xsave())) {
>> +               /*
>> +                * MSR IA32_XFD write follows after this XSAVE(S). So if a
>> +                * state component is in use, XFD should not be armed for
>> +                * current. But, for potential changes in the future,
>> +                * cross-check XINUSE and XFD values. If a XINUSE state
>> +                * is XFD-armed, the following XSAVE(S) does not save the
>> +                * state.
>> +                *
>> +                * Reference the shadow XFD value instead of reading the
>> +                * MSR.
>> +                */
>> +               if (xfd_capable() && boot_cpu_has(X86_FEATURE_XGETBV1)) {
>> +                       u64 current_xfd = (fpu->state_mask & xfd_capable()) ^ xfd_capable();
>> +
>> +                       WARN_ON_FPU(xgetbv(1) & current_xfd);
>> +               }
> 
> The code looks fine.  But, as usual, I hate the comment.  Maybe:
> 
> 		/*
> 		 * If XFD is armed for an xfeature, XSAVE* will not save
> 		 * its state.  Ensure XFD is clear for all features that
> 		 * are in use before XSAVE*.
> 		 */
> 
> BTW, the ->state_mask calculation is a little confusing to me.  I
> understand that fpu->state_mask shouldn't have any bits set that are
> unset in xgetbv(1).
> 
> This code seems to be asking the question: Are any dynamic features in
> their init state *and* XFD-armed?
> 
> Is it actually important to make sure that they are dynamic features?
> Is there *any* case where a feature (dynamic or not) can have XFD armed
> and be out of its init state?

In this AMX series, XFD is only used for the xstate buffer management. The
code is made in a such way that XFD and dynamic states are a bit coupled.

But I think XFD can be extended for other usages in the future. Then, yes.
(This warning is also for future code changes.)

So, reading the MSR is just simple and clean here, but it consumes cycles. Or,
a task may have a field for XFD value per se unless this conversion is
acceptable.

Thanks,
Chang