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

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

[Eric Biggers]

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@linutronix.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

Re: [5.2 regression] copy_fpstate_to_sigframe() change causing crash in 32-bit process

[Sebastian Andrzej Siewior]

On 2019-06-04 11:53:58 [-0700], Eric Biggers wrote:
> 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@linutronix.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.

This looked like a merge artifact and it has been confirmed as such. Now
you say that this was a needed piece of code. Interesting.
Is that wine process/testcase something you can share? I will try to
take a closer look.

Sebastian

Re: [5.2 regression] copy_fpstate_to_sigframe() change causing crash in 32-bit process

[Eric Biggers]

On Wed, Jun 05, 2019 at 04:04:05PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-06-04 11:53:58 [-0700], Eric Biggers wrote:
> > 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@linutronix.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.
> 
> This looked like a merge artifact and it has been confirmed as such. Now
> you say that this was a needed piece of code. Interesting.
> Is that wine process/testcase something you can share? I will try to
> take a closer look.
> 
> Sebastian

As I said, the commit looks broken to me.  save_fsave_header() reads from
tsk->thread.fpu.state.fxsave, which due to that commit isn't being updated with
the latest registers.  Am I missing something?  Note the comment you deleted:

	/* Update the thread's fxstate to save the fsave header. */

My test case was "run some Win32 game for a few minutes and see if it crashes"
so it's not really sharable, sorry.  But I expect it would be possible to write
a minimal test case, where a 32-bit process sends a signal to itself and checks
whether the i387 floating point stuff gets restored correctly afterwards.

- Eric

Re: [5.2 regression] copy_fpstate_to_sigframe() change causing crash in 32-bit process

[Sebastian Andrzej Siewior]

On 2019-06-05 10:32:57 [-0700], Eric Biggers wrote:
> As I said, the commit looks broken to me.  save_fsave_header() reads from
> tsk->thread.fpu.state.fxsave, which due to that commit isn't being updated with
> the latest registers.  Am I missing something?  Note the comment you deleted:

So if your system uses fxsr() then that function shouldn't matter. If
your system uses xsave() (which I believe it does) then the first
section is the "fxregs state" which is the same as in fxsr's case (see
struct xregs_state). So it shouldn't make a difference and that is why I
strongly assumed it is a miss-merge. However it makes a difference…

So the hunk at the end should make things work again (my FPU test case
passes). I don't know why we convert things forth and back in the signal
handler but I think something here is different for xsave's legacy area
vs fxsave.

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 060d6188b4533..c653c9920c5e0 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -62,16 +62,7 @@ static inline int save_fsave_header(struct task_struct *tsk, void __user *buf)
 		struct user_i387_ia32_struct env;
 		struct _fpstate_32 __user *fp = buf;
 
-		convert_from_fxsr(&env, tsk);
-
-		if (__copy_to_user(buf, &env, sizeof(env)) ||
-		    __put_user(xsave->i387.swd, &fp->status) ||
-		    __put_user(X86_FXSR_MAGIC, &fp->magic))
-			return -1;
-	} else {
-		struct fregs_state __user *fp = buf;
-		u32 swd;
-		if (__get_user(swd, &fp->swd) || __put_user(swd, &fp->status))
+		if (__put_user(X86_FXSR_MAGIC, &fp->magic))
 			return -1;
 	}
 
@@ -236,9 +227,6 @@ sanitize_restored_xstate(union fpregs_state *state,
 		 * reasons.
 		 */
 		xsave->i387.mxcsr &= mxcsr_feature_mask;
-
-		if (ia32_env)
-			convert_to_fxsr(&state->fxsave, ia32_env);
 	}
 }
 

> - Eric

Sebastian

[PATCH] x86/fpu: Update kernel's FPU state before using for the fsave header

[Sebastian Andrzej Siewior]

In commit

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

I removed the statement
|       if (ia32_fxstate)
|               copy_fxregs_to_kernel(fpu);

and argued that is was wrongly merged because the content was already
saved in kernel's state and the content.
This was wrong: It is required to write it back because it is only saved
on the user-stack and save_fsave_header() reads it from task's
FPU-state. I missed that part…

Save x87 FPU state unless thread's FPU registers are already up to date.

Fixes: 39388e80f9b0c ("x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe()")
Reported-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 060d6188b4533..0071b794ed193 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -62,6 +62,11 @@ static inline int save_fsave_header(struct task_struct *tsk, void __user *buf)
 		struct user_i387_ia32_struct env;
 		struct _fpstate_32 __user *fp = buf;
 
+		fpregs_lock();
+		if (!test_thread_flag(TIF_NEED_FPU_LOAD))
+			copy_fxregs_to_kernel(&tsk->thread.fpu);
+		fpregs_unlock();
+
 		convert_from_fxsr(&env, tsk);
 
 		if (__copy_to_user(buf, &env, sizeof(env)) ||
-- 
2.20.1

Re: [PATCH] x86/fpu: Update kernel's FPU state before using for the fsave header

[Eric Biggers]

On Fri, Jun 07, 2019 at 04:29:16PM +0200, Sebastian Andrzej Siewior wrote:
> In commit
> 
>   39388e80f9b0c ("x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe()")
> 
> I removed the statement
> |       if (ia32_fxstate)
> |               copy_fxregs_to_kernel(fpu);
> 
> and argued that is was wrongly merged because the content was already
> saved in kernel's state and the content.
> This was wrong: It is required to write it back because it is only saved
> on the user-stack and save_fsave_header() reads it from task's
> FPU-state. I missed that part…
> 
> Save x87 FPU state unless thread's FPU registers are already up to date.
> 
> Fixes: 39388e80f9b0c ("x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe()")
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/kernel/fpu/signal.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 060d6188b4533..0071b794ed193 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -62,6 +62,11 @@ static inline int save_fsave_header(struct task_struct *tsk, void __user *buf)
>  		struct user_i387_ia32_struct env;
>  		struct _fpstate_32 __user *fp = buf;
>  
> +		fpregs_lock();
> +		if (!test_thread_flag(TIF_NEED_FPU_LOAD))
> +			copy_fxregs_to_kernel(&tsk->thread.fpu);
> +		fpregs_unlock();
> +
>  		convert_from_fxsr(&env, tsk);
>  
>  		if (__copy_to_user(buf, &env, sizeof(env)) ||
> -- 
> 2.20.1
> 

Tested-by: Eric Biggers <ebiggers@kernel.org>

- Eric

[tip:x86/urgent] x86/fpu: Update kernel's FPU state before using for the fsave header

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  aab8445c4e1cceeb3f739352041ec1c2586bc923
Gitweb:     https://git.kernel.org/tip/aab8445c4e1cceeb3f739352041ec1c2586bc923
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Fri, 7 Jun 2019 16:29:16 +0200
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Sat, 8 Jun 2019 11:45:15 +0200

x86/fpu: Update kernel's FPU state before using for the fsave header

In commit

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

I removed the statement

|       if (ia32_fxstate)
|               copy_fxregs_to_kernel(fpu);

and argued that it was wrongly merged because the content was already
saved in kernel's state.

This was wrong: It is required to write it back because it is only
saved on the user-stack and save_fsave_header() reads it from task's
FPU-state. I missed that part…

Save x87 FPU state unless thread's FPU registers are already up to date.

Fixes: 39388e80f9b0c ("x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe()")
Reported-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Tested-by: Eric Biggers <ebiggers@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: kvm ML <kvm@vger.kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Rik van Riel <riel@surriel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190607142915.y52mfmgk5lvhll7n@linutronix.de
---
 arch/x86/kernel/fpu/signal.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 060d6188b453..0071b794ed19 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -62,6 +62,11 @@ static inline int save_fsave_header(struct task_struct *tsk, void __user *buf)
 		struct user_i387_ia32_struct env;
 		struct _fpstate_32 __user *fp = buf;
 
+		fpregs_lock();
+		if (!test_thread_flag(TIF_NEED_FPU_LOAD))
+			copy_fxregs_to_kernel(&tsk->thread.fpu);
+		fpregs_unlock();
+
 		convert_from_fxsr(&env, tsk);
 
 		if (__copy_to_user(buf, &env, sizeof(env)) ||