linux-kernel - [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190425173545.sogxyptbaqvoofm6@linutronix.de>
Date:   Thu, 25 Apr 2019 19:35:45 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     linux-kernel@...r.kernel.org
Cc:     x86@...nel.org, jannh@...gle.com, riel@...riel.com,
        dave.hansen@...el.com, mingo@...hat.com, bp@...e.de,
        Jason@...c4.com, luto@...nel.org, tglx@...utronix.de,
        rkrcmar@...hat.com, mingo@...nel.org, hpa@...or.com,
        kvm@...r.kernel.org, pbonzini@...hat.com,
        kurt.kanzenbach@...utronix.de
Subject: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE
 on sigentry

This commit reverts commit 04944b793e18e ("x86: xsave: set FP, SSE bits
in the xsave header in the user sigcontext"). The commit claims that it
is required for legacy applications but fails to explain why this is
needed and it is not obvious to me why the application would require the
FP/SSE state in the signal handler.

In the compacted form, the XSAVES may save only XMM+SSE but skip FP.
This is denoted by header->xfeatures = 6. The fastpath
(copy_fpregs_to_sigframe()) does that but _also_ initialises the FP
state (cwd to 0x37f, mxcsr as we do, remaining fields to 0).
The slowpath (copy_xstate_to_user()) leaves most of the FP untouched.
Only mxcsr and mxcsr_flags are set due to xfeatures_mxcsr_quirk().
Now that XFEATURE_MASK_FP is set unconditionally we load on return from
signal random garbage as the FP state.
This results in SIGFPE on one particular machine (the same testcase
(starting a java application) on older generations seems to work).

One way to fix this is not to unconditionally set XFEATURE_MASK_FPSSE
since it was never saved. This avoids loading garbage on the return
path.
We could also initialise the FP state in the xfeatures_mxcsr_quirk()
case like xsave does.

Reported-by: Kurt Kanzenbach <kurt.kanzenbach@...utronix.de>
Fixes: 69277c98f5eef ("x86/fpu: Always store the registers in copy_fpstate_to_sigframe()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
---
 arch/x86/kernel/fpu/signal.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7026f1c4e5e30..6c66203b19f3c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -81,7 +81,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 {
 	struct xregs_state __user *x = buf;
 	struct _fpx_sw_bytes *sw_bytes;
-	u32 xfeatures;
 	int err;

 	/* Setup the bytes not touched by the [f]xsave and reserved for SW. */
@@ -93,28 +92,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)

 	err |= __put_user(FP_XSTATE_MAGIC2,
 			  (__u32 __user *)(buf + fpu_user_xstate_size));
-
-	/*
-	 * Read the xfeatures which we copied (directly from the cpu or
-	 * from the state in task struct) to the user buffers.
-	 */
-	err |= __get_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
-
-	/*
-	 * For legacy compatible, we always set FP/SSE bits in the bit
-	 * vector while saving the state to the user context. This will
-	 * enable us capturing any changes(during sigreturn) to
-	 * the FP/SSE bits by the legacy applications which don't touch
-	 * xfeatures in the xsave header.
-	 *
-	 * xsave aware apps can change the xfeatures in the xsave
-	 * header as well as change any contents in the memory layout.
-	 * xrestore as part of sigreturn will capture all the changes.
-	 */
-	xfeatures |= XFEATURE_MASK_FPSSE;
-
-	err |= __put_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
-
 	return err;
 }

-- 
2.20.1