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

[Bae, Chang Seok]

On Jun 16, 2021, at 11:12, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> On 6/16/21 9:27 AM, Dave Hansen wrote:
>> On 5/23/21 12:32 PM, Chang S. Bae wrote:
>>> @@ -571,6 +612,8 @@ static inline void switch_fpu_finish(struct fpu *new_fpu)
>>> 
>>> 	set_thread_flag(TIF_NEED_FPU_LOAD);
>>> 
>>> +	xfd_switch(old_fpu, new_fpu);
>> 
>> This seems fragile.
>> 
>> XSAVE* will decline to write out state for feature i when XFD[i]=1 and
>> will instead write out the init state.  That means that, at this point,
>> we switch XFD and yet leave state for feature i in place.
>> 
>> That means this *MUST* come before any copy_fpregs_to_fpstate() occurs.
>> 
>> Could we please add some FPU_WARN_ON() checks to ensure that no XSAVE*
>> is ever performed with XINUSE=1 *and* XFD armed?

So, xfd_switch() is after copy_fpregs_to_fpstate():
 
__switch_to()
--> switch_fpu_prepare() --> copy_fpregs_to_fpstate()
--> switch_fpu_finish() --> xfd_switch()
 
I don't see someone intentionally flip this order.

Reading XINUSE via XGETBV is cheap but not free. I don't know spending a
hundred cycles for this WARN is big deal but this is one of the most
performance-critical paths.

> Wait, really?  I somehow thought that XSAVE* would write out the actual
> state even if XFD=1.

No, in section 3.2.6 Extended Feature Disable (XFD) of [1]:

    "If XSAVE, XSAVEC, XSAVEOPT, or XSAVES is saving the state component i,
     the instruction does not generate #NM when XCR0[i] = IA32_XFD[i] = 1;
     instead, it saves bit i of XSTATE_BV field of the XSAVE header as 0 (
     indicating that the state component is in its initialized state). With
     the exception of XSAVE, no data is saved for the state component (XSAVE
     saves the initial value of the state component; for XTILEDATA, this is
     all zeroes)."

> This seems like it's asking for some kind of bug.

[1]: Intel Architecture Instruction Set Extensions Programming Reference, https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html