Re: [tip: x86/fpu] x86/fpu/xstate: Define new functions for clearing fpregs and xstates

["Yu, Yu-cheng" <yu-cheng.yu@intel.com>]

On 5/24/2021 9:34 AM, Andy Lutomirski wrote:
> On Sat, May 16, 2020 at 8:10 AM tip-bot2 for Fenghua Yu
> <tip-bot2@linutronix.de> wrote:
>>
>> The following commit has been merged into the x86/fpu branch of tip:
>>
>> Commit-ID:     b860eb8dce5906b14e3a7f3c771e0b3d6ef61b94
>> Gitweb:        https://git.kernel.org/tip/b860eb8dce5906b14e3a7f3c771e0b3d6ef61b94
>> Author:        Fenghua Yu <fenghua.yu@intel.com>
>> AuthorDate:    Tue, 12 May 2020 07:54:39 -07:00
>> Committer:     Borislav Petkov <bp@suse.de>
>> CommitterDate: Wed, 13 May 2020 13:41:50 +02:00
>>
>> x86/fpu/xstate: Define new functions for clearing fpregs and xstates
> 
> syzbot says this is busted.  I've made no effort to identify the
> precise bug that is making syzbot complain, but:
> 
>>   /*
>> - * Clear FPU registers by setting them up from
>> - * the init fpstate:
>> + * Clear FPU registers by setting them up from the init fpstate.
>> + * Caller must do fpregs_[un]lock() around it.
>>    */
>> -static inline void copy_init_fpstate_to_fpregs(void)
>> +static inline void copy_init_fpstate_to_fpregs(u64 features_mask)
>>   {
>> -       fpregs_lock();
>> -
> 
> 
> 
>>          if (use_xsave())
>> -               copy_kernel_to_xregs(&init_fpstate.xsave, -1);
>> +               copy_kernel_to_xregs(&init_fpstate.xsave, features_mask);
>>          else if (static_cpu_has(X86_FEATURE_FXSR))
>>                  copy_kernel_to_fxregs(&init_fpstate.fxsave);
>>          else
>> @@ -307,9 +305,6 @@ static inline void copy_init_fpstate_to_fpregs(void)
>>
>>          if (boot_cpu_has(X86_FEATURE_OSPKE))
>>                  copy_init_pkru_to_fpregs();
> 
> if (boot_cpu_has(X86_FEATURE_OSPKE) && (features_mask & PKRU)), perhaps?
> 
>> -
>> -       fpregs_mark_activate();
>> -       fpregs_unlock();
>>   }
>>
>>   /*
>> @@ -318,18 +313,40 @@ static inline void copy_init_fpstate_to_fpregs(void)
>>    * Called by sys_execve(), by the signal handler code and by various
>>    * error paths.
>>    */
>> -void fpu__clear(struct fpu *fpu)
>> +static void fpu__clear(struct fpu *fpu, bool user_only)
>>   {
>> -       WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
>> +       WARN_ON_FPU(fpu != &current->thread.fpu);
>>
>> -       fpu__drop(fpu);
>> +       if (!static_cpu_has(X86_FEATURE_FPU)) {
>> +               fpu__drop(fpu);
>> +               fpu__initialize(fpu);
>> +               return;
>> +       }
>>
>> -       /*
>> -        * Make sure fpstate is cleared and initialized.
>> -        */
>> -       fpu__initialize(fpu);
>> -       if (static_cpu_has(X86_FEATURE_FPU))
>> -               copy_init_fpstate_to_fpregs();
>> +       fpregs_lock();
>> +
>> +       if (user_only) {
>> +               if (!fpregs_state_valid(fpu, smp_processor_id()) &&
>> +                   xfeatures_mask_supervisor())
>> +                       copy_kernel_to_xregs(&fpu->state.xsave,
>> +                                            xfeatures_mask_supervisor());
> 
> This looks correct to me.
> 
> So I'm guessing that syzbot may have misattributed the problem.  But
> we definitely need to clean up the XRSTOR #GP handling before CET
> lands.
> 

 From the crash dump, the system is doing syscall_exit_to_user_mode() 
for __x64_sys_futex().  The futex syscall does not seem to modify 
xstates, but upon returning to user mode, XRSTORS gets a GP.  Can this 
be some memory corruption?  fpu__clear() is merely helping to clear the 
mess and seems to be innocent.

I also run the syz repro on my Tiger Lake machine, and it only produces 
segfaults (no Bad FPU state, etc.).

Yu-cheng