linux-kernel - [5.2 regression] copy_fpstate_to_sigframe() change causing crash in 32-bit process

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: <20190604185358.GA820@sol.localdomain>
Date:   Tue, 4 Jun 2019 11:53:58 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Borislav Petkov <bp@...e.de>, Dave Hansen <dave.hansen@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Jann Horn <jannh@...gle.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        kvm ML <kvm@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Rik van Riel <riel@...riel.com>, x86-ml <x86@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: [5.2 regression] copy_fpstate_to_sigframe() change causing crash in
 32-bit process

On latest Linus' tree I'm getting a crash in a 32-bit Wine process.

I bisected it to the following commit:

commit 39388e80f9b0c3788bfb6efe3054bdce0c3ead45
Author: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Date:   Wed Apr 3 18:41:35 2019 +0200

    x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe()

Reverting the commit by applying the following diff makes the problem go away.

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 5a8d118bc423e..ed16a24aab497 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -157,6 +157,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
  */
 int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 {
+	struct fpu *fpu = &current->thread.fpu;
 	struct task_struct *tsk = current;
 	int ia32_fxstate = (buf != buf_fx);
 	int ret;
@@ -202,6 +203,10 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 		return -EFAULT;
 	}
 
+	/* Update the thread's fxstate to save the fsave header. */
+	if (ia32_fxstate)
+		copy_fxregs_to_kernel(fpu);
+
 	/* Save the fsave header for the 32-bit frames. */
 	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))
 		return -1;

Apparently the problem is that save_fsave_header() assumes the registers have
been saved to fpu->state.fxsave, yet the code that does so was removed.

Note, bisection was not straightforward because there was another bug also
causing a crash temporarily introduced during the FPU code rework: commit
39ea9baffda9 ("x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()")
forgot to call fpstate_init() on the temporary 'state' buffer, so
XCOMP_BV_COMPACTED_FORMAT was never set, causing xrstors to fail.  But that bug
went away in later commits.

- Eric