Re: [syzbot] WARNING in ex_handler_fprestore

[Yu, Yu-cheng]

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;
> }