Re: [syzbot] WARNING in ex_handler_fprestore

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

On 5/27/2021 9:49 AM, Thomas Gleixner wrote:
> On Thu, May 27 2021 at 00:03, Thomas Gleixner wrote:
>> I decoded it fully by now and will send out coherent info (and hopefully
>> a patch) tomorrow with brain awake. Time for bed here...
> 
> Not yet there with a patch, but I have a trivial C reproducer. See
> below.
> 
> The failure mode is:
> 
>      signal to task
>        sigaction_handler()
>          wreckage xsave.header.reserved[0]
>          sig_return()
>            restore_fpu_from_sigframe()
>              try: XRSTOR from user -> #GP
>              copy_from_user(fpstate, task->fpu.state.xsave);
>              validate(task->fpu.state.xsave) -> fail
>              fpu__clear_user_states()
>                reinits FPU hardware but task state is still wreckaged

In fpu__clear_user_states(), maybe can we clear xstate_header.reserved[] 
as well?  Or do we want to check the user buffer first before copy it?

>            signal_fault()
>        sigsegv_handler()
>          sig_return()
>            restore_fpu_from_sigframe()
>              try: XRSTOR from user -> success
> 
>       schedule()
>         xsave()
> 
>       other task runs on same CPU so fpu state is not longer clean
> 
>       wakeup()
>       exit_to_user()
>        xrstor() -> #GP because the header is still borked.
> 
> XSAVE does not clear the header.reserved fields....
> 
> The original code kinda worked because fpu__clear() reinitialized the
> task fpu state, but as this code is preemptible the same issue can
> happen pre 5.8 as well if I'm not missing something. I'll verify that
> after dinner.
> 
> The commit in question just made it trivial to trigger because it keeps
> the broken task fpu state around.
> 
> The whole slow path is broken in disgusting ways. I have no good idea
> yet how to fix that proper and in a way which can be backported easily.
> 
> Thanks,
> 
>          tglx
> ---
> #define _GNU_SOURCE
> 
> #include <errno.h>
> #include <pthread.h>
> #include <signal.h>
> #include <stdlib.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> #include <sched.h>
> 
> static void sig_handler(int sig, siginfo_t *info, void *__ctxp)
> {
> 	ucontext_t *uctx = __ctxp;
> 	mcontext_t *mctx = &uctx->uc_mcontext;
> 	unsigned long *p = (unsigned long *) mctx->fpregs;
> 
> 	/* Wreckage xsave.header.reserved[0] */
> 	p[66] = 0xfffffff;
> }
> 
> static void sigsegv(int sig)
> {
> }
> 
> int main(void)
> {
> 	struct sigaction sa;
> 	cpu_set_t set;
> 
> 	memset(&sa, 0, sizeof(sa));
> 	sa.sa_sigaction = sig_handler;
> 	sa.sa_flags = SA_SIGINFO;
> 	sigemptyset(&sa.sa_mask);
> 	if (sigaction(SIGUSR1, &sa, 0))
> 		exit(1);
> 
> 	signal(SIGSEGV, sigsegv);
> 
> 	CPU_ZERO(&set);
> 	CPU_SET(0, &set);
> 
> 	sched_setaffinity(getpid(), sizeof(set), &set);
> 
> 	kill(0, SIGUSR1);
> 	fork();
> 	sleep(1);
> 
> 	return 0;
> }
>