Bug 205663

Summary:	AVX register corruption from signal delivery
Product:	Platform Specific/Hardware	Reporter:	Austin Clements (austin)
Component:	x86-64	Assignee:	platform_x86_64 (platform_x86_64)
Status:	RESOLVED CODE_FIX
Severity:	normal	CC:	alexander, anthony.louis.eden, bp, brho, cheetah-kbt, cks-kernelbugs, code, dvyukov, fweimer, gmarwan.berkani, grizzlyuser, ian, jbuchert+kbugs, josharian, ncopa, nrain.season2y, paul.richards, synixaco
Priority:	P1
Hardware:	x86-64
OS:	Linux
Kernel Version:	5.2	Subsystem:
Regression:	No	Bisected commit-id:
Attachments:	Reproducer

Description Austin Clements 2019-11-26 16:52:07 UTC

Created attachment 286073 [details]
Reproducer

When a signal is delivered that must fault in user stack pages in order to XSAVE the signal context, AVX YMM registers may be corrupted on return from the signal handler.

To reproduce, build the attached program with "gcc -pthread test.c" and run it on a 5.2 or later kernel compiled with GCC 9 (if the kernel is compiled with GCC 8, it does *not* reproduce).

Actual result: On v5.2 and later kernels, it will fail quickly with a corrupted value in an AVX register.

Expected result: No register corruption, in which case it will exit successfully after one minute.

The test program runs a thread doing a simple AVX computation and bombards it with signals. The signal handler itself is a no-op. The program also MADV_DONTNEEDs the signal handler's stack repeatedly, since forcing copy_fpstate_to_sigframe to fault in the user stack pages is necessary to cause the corruption. Finally, it runs several instances of the process in order to force context switching, which also appears to be necessary.

We've bisected this issue to commit d9c9ce34ed5c ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails"), which was released in the v5.2 kernel. I've also been able to reproduce this at b81ff1013eb8 ("x86/fpu: Use fault_in_pages_writeable() for pre-faulting"), which fixed a bug in commit d9c9ce34ed5c, and at v5.2, v5.3 and v5.4 (always compiling with GCC 9).

For reference, this bug was originally reported as memory corruption in Go. We tracked it this far in https://github.com/golang/go/issues/35326.

Comment 1 Ian Lance Taylor 2019-11-27 00:00:13 UTC

FYI, as mentioned, this only happens when the kernel is compiled with GCC 9.  The file that matters is arch/x86/kernel/fpu/signal.c.  The difference seems to be that with GCC 9 the address of the thread-local variable used for test_thread_flag is cached across the function.  With GCC 8 the address of the variable is reloaded each time the variable is referenced.  If the retry loop that calls fault_in_pages_writeable can cause a change in kernel threads, then with GCC 9 the call to test_thread_flag in the second and subsequent loop iterations may look at the wrong thread flag.  I don't know if this is the problem but it seems worth mentioning.

Comment 2 Austin Clements 2019-11-27 18:16:25 UTC

I can confirm that the patch posted by Sebastian Andrzej Siewior at https://lkml.org/lkml/2019/11/27/304 fixes the issue both in our C reproducer and in our original Go reproducer. (Sorry, I'm not subscribed to LKML, so I can't reply there, and I'm on an airplane, so it's hard to get subscribed :)

Regarding the question about the "Debugged-by" line in the patch, debugging was a joint effort between myself (Austin Clements <austin@google.com>), David Chase <drchase@golang.org>, and Ian Lance Taylor <ian@airs.com>.

Comment 3 Borislav Petkov 2019-11-28 09:59:14 UTC

Fix queued on its way upstream:

https://git.kernel.org/tip/59c4bd853abcea95eccc167a7d7fd5f1a5f47b98

Thanks to everyone involved!