Re: [syzbot] WARNING in ex_handler_fprestore

From: Yu, Yu-cheng
Date: Thu May 27 2021 - 14:59:39 EST


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