linux-kernel - Re: [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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Jun 2019 19:30:39 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Eric Biggers <ebiggers@...nel.org>
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: Re: [5.2 regression] copy_fpstate_to_sigframe() change causing crash
 in 32-bit process

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