[patch 00/10] x86/fpu: Clean up error handling in sigframe related code

[patch 00/10] x86/fpu: Clean up error handling in sigframe related code

[Thomas Gleixner]

A recent discussion [1] about hardware poisoning unearthed some short
comings in the error handling of the sigframe related FPU code:

  - The error exit for exceptions other than #PF is obfuscated

  - The error code return values of the various functions are pointless
    because all callers just care about success or failure and the error
    codes are never propagated to user space.

  - Some of the buffer clearing happens needlessly inside of page fault
    disabled regions.

The following series cleans this up. As a side effect the resulting text
size of fpu/signal.o shrinks by about 150 bytes.

It's also available in git:

    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git x86/fpu

Thanks,

	tglx

[1] https://lore.kernel.org/r/87r1edgs2w.ffs@tglx

---
 ia32/ia32_signal.c         |   14 ++--
 include/asm/fpu/internal.h |   21 ++----
 kernel/fpu/signal.c        |  145 +++++++++++++++++++++++----------------------
 kernel/signal.c            |   18 ++---
 4 files changed, 98 insertions(+), 100 deletions(-)

[patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

FPU restore from a signal frame can trigger various exceptions. The
exceptions are caught with an exception table entry. The handler of this
entry sets the error return value to the negated exception number.

Any other exception than #PF is fatal and recovery is not possible. This
relies on the fact that the #PF exception number is the same as EFAULT, but
that's not really obvious.

Check the error code for -X86_TRAP_PF instead of checking it for -EFAULT to
make it clear how that works.

There is still confusion due to the return code conversion which will be
cleaned up separately.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -13,6 +13,7 @@
 #include <asm/fpu/xstate.h>
 
 #include <asm/sigframe.h>
+#include <asm/trapnr.h>
 #include <asm/trace/fpu.h>
 
 static struct _fpx_sw_bytes fx_sw_reserved __ro_after_init;
@@ -275,7 +276,7 @@ static int restore_fpregs_from_user(void
 		fpregs_unlock();
 
 		/* Try to handle #PF, but anything else is fatal. */
-		if (ret != -EFAULT)
+		if (ret != -X86_TRAP_PF)
 			return -EINVAL;
 
 		ret = fault_in_pages_readable(buf, size);

[patch 02/10] x86/fpu/signal: Move header zeroing out of xsave_to_user_sigframe()

[Thomas Gleixner]

There is no reason to have the header zeroing in the pagefault disabled
region. Do it upfront once.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/fpu/internal.h |   17 ++++++-----------
 arch/x86/kernel/fpu/signal.c        |   12 ++++++++++++
 2 files changed, 18 insertions(+), 11 deletions(-)

--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -323,9 +323,12 @@ static inline void os_xrstor(struct xreg
  * We don't use modified optimization because xrstor/xrstors might track
  * a different application.
  *
- * We don't use compacted format xsave area for
- * backward compatibility for old applications which don't understand
- * compacted format of xsave area.
+ * We don't use compacted format xsave area for backward compatibility for
+ * old applications which don't understand the compacted format of the
+ * xsave area.
+ *
+ * The caller has to zero buf::header before calling this because XSAVE*
+ * does not touch the reserved fields in the header.
  */
 static inline int xsave_to_user_sigframe(struct xregs_state __user *buf)
 {
@@ -339,14 +342,6 @@ static inline int xsave_to_user_sigframe
 	u32 hmask = mask >> 32;
 	int err;
 
-	/*
-	 * Clear the xsave header first, so that reserved fields are
-	 * initialized to zero.
-	 */
-	err = __clear_user(&buf->header, sizeof(buf->header));
-	if (unlikely(err))
-		return -EFAULT;
-
 	stac();
 	XSTATE_OP(XSAVE, buf, lmask, hmask, err);
 	clac();
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -189,6 +189,18 @@ int copy_fpstate_to_sigframe(void __user
 
 	if (!access_ok(buf, size))
 		return -EACCES;
+
+	if (use_xsave()) {
+		struct xregs_state __user *xbuf = buf_fx;
+
+		/*
+		 * Clear the xsave header first, so that reserved fields are
+		 * initialized to zero.
+		 */
+		ret = __clear_user(&xbuf->header, sizeof(xbuf->header));
+		if (unlikely(ret))
+			return ret;
+	}
 retry:
 	/*
 	 * Load the FPU registers if they are not valid for the current task.

[patch 03/10] x86/fpu/signal: Move xstate clearing out of copy_fpregs_to_sigframe()

[Thomas Gleixner]

When the direct saving of the FPU registers to the user space sigframe
fails, copy_fpregs_to_sigframe() attempts to clear the user buffer.

The most likely reason for such a fail is a page fault. As
copy_fpregs_to_sigframe() is invoked with pagefaults disabled the chance
that __clear_user() succeeds is minuscule.

Move the clearing out into the caller which replaces the
fault_in_pages_writeable() in that error handling path.

The return value confusion will be cleaned up separately.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c |   19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -136,18 +136,12 @@ static inline int save_xstate_epilog(voi
 
 static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 {
-	int err;
-
 	if (use_xsave())
-		err = xsave_to_user_sigframe(buf);
-	else if (use_fxsr())
-		err = fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
+		return xsave_to_user_sigframe(buf);
+	if (use_fxsr())
+		return fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
 	else
-		err = fnsave_to_user_sigframe((struct fregs_state __user *) buf);
-
-	if (unlikely(err) && __clear_user(buf, fpu_user_xstate_size))
-		err = -EFAULT;
-	return err;
+		return fnsave_to_user_sigframe((struct fregs_state __user *) buf);
 }
 
 /*
@@ -218,9 +212,10 @@ int copy_fpstate_to_sigframe(void __user
 	fpregs_unlock();
 
 	if (ret) {
-		if (!fault_in_pages_writeable(buf_fx, fpu_user_xstate_size))
+		if (!__clear_user(buf_fx, fpu_user_xstate_size) &&
+		    ret == -X86_TRAP_PF)
 			goto retry;
-		return -EFAULT;
+		return -1;
 	}
 
 	/* Save the fsave header for the 32-bit frames. */

[patch 04/10] x86/fpu/signal: Change return type of copy_fpstate_to_sigframe() to boolean

[Thomas Gleixner]

None of the call sites cares about the actual return code. Change the
return type to boolean and return 'true' on success.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/ia32/ia32_signal.c         |    4 ++--
 arch/x86/include/asm/fpu/internal.h |    2 +-
 arch/x86/kernel/fpu/signal.c        |   20 ++++++++++----------
 arch/x86/kernel/signal.c            |    4 +---
 4 files changed, 14 insertions(+), 16 deletions(-)

--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -220,8 +220,8 @@ static void __user *get_sigframe(struct
 
 	sp = fpu__alloc_mathframe(sp, 1, &fx_aligned, &math_size);
 	*fpstate = (struct _fpstate_32 __user *) sp;
-	if (copy_fpstate_to_sigframe(*fpstate, (void __user *)fx_aligned,
-				     math_size) < 0)
+	if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)fx_aligned,
+				      math_size))
 		return (void __user *) -1L;
 
 	sp -= frame_size;
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -391,7 +391,7 @@ static inline void restore_fpregs_from_f
 	__restore_fpregs_from_fpstate(fpstate, xfeatures_mask_fpstate());
 }
 
-extern int copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size);
+extern bool copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size);
 
 /*
  * FPU context switch related helper methods:
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -165,7 +165,7 @@ static inline int copy_fpregs_to_sigfram
  * For [f]xsave state, update the SW reserved fields in the [f]xsave frame
  * indicating the absence/presence of the extended state to the user.
  */
-int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
+bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 {
 	struct task_struct *tsk = current;
 	int ia32_fxstate = (buf != buf_fx);
@@ -176,13 +176,14 @@ int copy_fpstate_to_sigframe(void __user
 
 	if (!static_cpu_has(X86_FEATURE_FPU)) {
 		struct user_i387_ia32_struct fp;
+
 		fpregs_soft_get(current, NULL, (struct membuf){.p = &fp,
 						.left = sizeof(fp)});
-		return copy_to_user(buf, &fp, sizeof(fp)) ? -EFAULT : 0;
+		return !copy_to_user(buf, &fp, sizeof(fp));
 	}
 
 	if (!access_ok(buf, size))
-		return -EACCES;
+		return false;
 
 	if (use_xsave()) {
 		struct xregs_state __user *xbuf = buf_fx;
@@ -191,9 +192,8 @@ int copy_fpstate_to_sigframe(void __user
 		 * Clear the xsave header first, so that reserved fields are
 		 * initialized to zero.
 		 */
-		ret = __clear_user(&xbuf->header, sizeof(xbuf->header));
-		if (unlikely(ret))
-			return ret;
+		if (__clear_user(&xbuf->header, sizeof(xbuf->header)))
+			return false;
 	}
 retry:
 	/*
@@ -215,17 +215,17 @@ int copy_fpstate_to_sigframe(void __user
 		if (!__clear_user(buf_fx, fpu_user_xstate_size) &&
 		    ret == -X86_TRAP_PF)
 			goto retry;
-		return -1;
+		return false;
 	}
 
 	/* Save the fsave header for the 32-bit frames. */
 	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))
-		return -1;
+		return false;
 
 	if (use_fxsr() && save_xstate_epilog(buf_fx, ia32_fxstate))
-		return -1;
+		return false;
 
-	return 0;
+	return true;
 }
 
 static int __restore_fpregs_from_user(void __user *buf, u64 xrestore,
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -244,7 +244,6 @@ get_sigframe(struct k_sigaction *ka, str
 	unsigned long math_size = 0;
 	unsigned long sp = regs->sp;
 	unsigned long buf_fx = 0;
-	int ret;
 
 	/* redzone */
 	if (IS_ENABLED(CONFIG_X86_64))
@@ -292,8 +291,7 @@ get_sigframe(struct k_sigaction *ka, str
 	}
 
 	/* save i387 and extended state */
-	ret = copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size);
-	if (ret < 0)
+	if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size))
 		return (void __user *)-1L;
 
 	return (void __user *)sp;

[patch 05/10] x86/fpu/signal: Change return type of copy_fpregs_to_sigframe() helpers to boolean

[Thomas Gleixner]

Now that copy_fpregs_to_sigframe() returns boolean the individual return
codes in the related helper functions do not make sense anymore. Change
them to return boolean success/fail.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -65,7 +65,7 @@ static inline int check_xstate_in_sigfra
 /*
  * Signal frame handlers.
  */
-static inline int save_fsave_header(struct task_struct *tsk, void __user *buf)
+static inline bool save_fsave_header(struct task_struct *tsk, void __user *buf)
 {
 	if (use_fxsr()) {
 		struct xregs_state *xsave = &tsk->thread.fpu.state.xsave;
@@ -82,18 +82,19 @@ static inline int save_fsave_header(stru
 		if (__copy_to_user(buf, &env, sizeof(env)) ||
 		    __put_user(xsave->i387.swd, &fp->status) ||
 		    __put_user(X86_FXSR_MAGIC, &fp->magic))
-			return -1;
+			return false;
 	} else {
 		struct fregs_state __user *fp = buf;
 		u32 swd;
+
 		if (__get_user(swd, &fp->swd) || __put_user(swd, &fp->status))
-			return -1;
+			return false;
 	}
 
-	return 0;
+	return true;
 }
 
-static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
+static inline bool save_xstate_epilog(void __user *buf, int ia32_frame)
 {
 	struct xregs_state __user *x = buf;
 	struct _fpx_sw_bytes *sw_bytes;
@@ -131,7 +132,7 @@ static inline int save_xstate_epilog(voi
 
 	err |= __put_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
 
-	return err;
+	return !err;
 }
 
 static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
@@ -219,10 +220,10 @@ bool copy_fpstate_to_sigframe(void __use
 	}
 
 	/* Save the fsave header for the 32-bit frames. */
-	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))
+	if ((ia32_fxstate || !use_fxsr()) && !save_fsave_header(tsk, buf))
 		return false;
 
-	if (use_fxsr() && save_xstate_epilog(buf_fx, ia32_fxstate))
+	if (use_fxsr() && !save_xstate_epilog(buf_fx, ia32_fxstate))
 		return false;
 
 	return true;

[patch 06/10] x86/signal: Change return type of restore_sigcontext() to boolean

[Thomas Gleixner]

None of the call sites cares about the return code. All they are interested
in is success or fail.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/ia32/ia32_signal.c |   12 ++++++------
 arch/x86/kernel/signal.c    |   18 +++++++++---------
 2 files changed, 15 insertions(+), 15 deletions(-)

--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -57,8 +57,8 @@ static inline void reload_segments(struc
 /*
  * Do a signal return; undo the signal stack.
  */
-static int ia32_restore_sigcontext(struct pt_regs *regs,
-				   struct sigcontext_32 __user *usc)
+static bool ia32_restore_sigcontext(struct pt_regs *regs,
+				    struct sigcontext_32 __user *usc)
 {
 	struct sigcontext_32 sc;
 
@@ -66,7 +66,7 @@ static int ia32_restore_sigcontext(struc
 	current->restart_block.fn = do_no_restart_syscall;
 
 	if (unlikely(copy_from_user(&sc, usc, sizeof(sc))))
-		return -EFAULT;
+		return false;
 
 	/* Get only the ia32 registers. */
 	regs->bx = sc.bx;
@@ -94,7 +94,7 @@ static int ia32_restore_sigcontext(struc
 	 * normal case.
 	 */
 	reload_segments(&sc);
-	return fpu__restore_sig(compat_ptr(sc.fpstate), 1);
+	return !fpu__restore_sig(compat_ptr(sc.fpstate), 1);
 }
 
 COMPAT_SYSCALL_DEFINE0(sigreturn)
@@ -111,7 +111,7 @@ COMPAT_SYSCALL_DEFINE0(sigreturn)
 
 	set_current_blocked(&set);
 
-	if (ia32_restore_sigcontext(regs, &frame->sc))
+	if (!ia32_restore_sigcontext(regs, &frame->sc))
 		goto badframe;
 	return regs->ax;
 
@@ -135,7 +135,7 @@ COMPAT_SYSCALL_DEFINE0(rt_sigreturn)
 
 	set_current_blocked(&set);
 
-	if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
+	if (!ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		goto badframe;
 
 	if (compat_restore_altstack(&frame->uc.uc_stack))
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -79,9 +79,9 @@ static void force_valid_ss(struct pt_reg
 # define CONTEXT_COPY_SIZE	sizeof(struct sigcontext)
 #endif
 
-static int restore_sigcontext(struct pt_regs *regs,
-			      struct sigcontext __user *usc,
-			      unsigned long uc_flags)
+static bool restore_sigcontext(struct pt_regs *regs,
+			       struct sigcontext __user *usc,
+			       unsigned long uc_flags)
 {
 	struct sigcontext sc;
 
@@ -89,7 +89,7 @@ static int restore_sigcontext(struct pt_
 	current->restart_block.fn = do_no_restart_syscall;
 
 	if (copy_from_user(&sc, usc, CONTEXT_COPY_SIZE))
-		return -EFAULT;
+		return false;
 
 #ifdef CONFIG_X86_32
 	set_user_gs(regs, sc.gs);
@@ -136,8 +136,8 @@ static int restore_sigcontext(struct pt_
 		force_valid_ss(regs);
 #endif
 
-	return fpu__restore_sig((void __user *)sc.fpstate,
-			       IS_ENABLED(CONFIG_X86_32));
+	return !fpu__restore_sig((void __user *)sc.fpstate,
+				 IS_ENABLED(CONFIG_X86_32));
 }
 
 static __always_inline int
@@ -641,7 +641,7 @@ SYSCALL_DEFINE0(sigreturn)
 	 * x86_32 has no uc_flags bits relevant to restore_sigcontext.
 	 * Save a few cycles by skipping the __get_user.
 	 */
-	if (restore_sigcontext(regs, &frame->sc, 0))
+	if (!restore_sigcontext(regs, &frame->sc, 0))
 		goto badframe;
 	return regs->ax;
 
@@ -669,7 +669,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
 
 	set_current_blocked(&set);
 
-	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
+	if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
 		goto badframe;
 
 	if (restore_altstack(&frame->uc.uc_stack))
@@ -927,7 +927,7 @@ COMPAT_SYSCALL_DEFINE0(x32_rt_sigreturn)
 
 	set_current_blocked(&set);
 
-	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
+	if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
 		goto badframe;
 
 	if (compat_restore_altstack(&frame->uc.uc_stack))

[patch 07/10] x86/fpu/signal: Change return type of fpu__restore_sig() to boolean

[Thomas Gleixner]

None of the call sites cares about the error code. All they need to know is
whether the function succeeded or not.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/ia32/ia32_signal.c         |    2 +-
 arch/x86/include/asm/fpu/internal.h |    2 +-
 arch/x86/kernel/fpu/signal.c        |   22 ++++++++++------------
 arch/x86/kernel/signal.c            |    4 ++--
 4 files changed, 14 insertions(+), 16 deletions(-)

--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -94,7 +94,7 @@ static bool ia32_restore_sigcontext(stru
 	 * normal case.
 	 */
 	reload_segments(&sc);
-	return !fpu__restore_sig(compat_ptr(sc.fpstate), 1);
+	return fpu__restore_sig(compat_ptr(sc.fpstate), 1);
 }
 
 COMPAT_SYSCALL_DEFINE0(sigreturn)
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -26,7 +26,7 @@
 /*
  * High level FPU state handling functions:
  */
-extern int  fpu__restore_sig(void __user *buf, int ia32_frame);
+extern bool fpu__restore_sig(void __user *buf, int ia32_frame);
 extern void fpu__drop(struct fpu *fpu);
 extern void fpu__clear_user_states(struct fpu *fpu);
 extern int  fpu__exception_code(struct fpu *fpu, int trap_nr);
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -434,17 +434,17 @@ static inline int xstate_sigframe_size(v
 /*
  * Restore FPU state from a sigframe:
  */
-int fpu__restore_sig(void __user *buf, int ia32_frame)
+bool fpu__restore_sig(void __user *buf, int ia32_frame)
 {
 	unsigned int size = xstate_sigframe_size();
 	struct fpu *fpu = &current->thread.fpu;
 	void __user *buf_fx = buf;
 	bool ia32_fxstate = false;
-	int ret;
+	bool success = false;
 
 	if (unlikely(!buf)) {
 		fpu__clear_user_states(fpu);
-		return 0;
+		return true;
 	}
 
 	ia32_frame &= (IS_ENABLED(CONFIG_X86_32) ||
@@ -460,23 +460,21 @@ int fpu__restore_sig(void __user *buf, i
 		ia32_fxstate = true;
 	}
 
-	if (!access_ok(buf, size)) {
-		ret = -EACCES;
+	if (!access_ok(buf, size))
 		goto out;
-	}
 
 	if (!IS_ENABLED(CONFIG_X86_64) && !cpu_feature_enabled(X86_FEATURE_FPU)) {
-		ret = fpregs_soft_set(current, NULL, 0,
-				      sizeof(struct user_i387_ia32_struct),
-				      NULL, buf);
+		success = !fpregs_soft_set(current, NULL, 0,
+					   sizeof(struct user_i387_ia32_struct),
+					   NULL, buf);
 	} else {
-		ret = __fpu_restore_sig(buf, buf_fx, ia32_fxstate);
+		success = !__fpu_restore_sig(buf, buf_fx, ia32_fxstate);
 	}
 
 out:
-	if (unlikely(ret))
+	if (unlikely(!success))
 		fpu__clear_user_states(fpu);
-	return ret;
+	return success;
 }
 
 unsigned long
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -136,8 +136,8 @@ static bool restore_sigcontext(struct pt
 		force_valid_ss(regs);
 #endif
 
-	return !fpu__restore_sig((void __user *)sc.fpstate,
-				 IS_ENABLED(CONFIG_X86_32));
+	return fpu__restore_sig((void __user *)sc.fpstate,
+			       IS_ENABLED(CONFIG_X86_32));
 }
 
 static __always_inline int

[patch 08/10] x86/fpu/signal: Change return type of __fpu_restore_sig() to boolean

[Thomas Gleixner]

Now that fpu__restore_sig() returns a boolean get rid of the individual
error codes in __fpu_restore_sig() as well.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -310,8 +310,8 @@ static int restore_fpregs_from_user(void
 	return 0;
 }
 
-static int __fpu_restore_sig(void __user *buf, void __user *buf_fx,
-			     bool ia32_fxstate)
+static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
+			      bool ia32_fxstate)
 {
 	int state_size = fpu_kernel_xstate_size;
 	struct task_struct *tsk = current;
@@ -319,14 +319,14 @@ static int __fpu_restore_sig(void __user
 	struct user_i387_ia32_struct env;
 	u64 user_xfeatures = 0;
 	bool fx_only = false;
-	int ret;
+	bool success;
+
 
 	if (use_xsave()) {
 		struct _fpx_sw_bytes fx_sw_user;
 
-		ret = check_xstate_in_sigframe(buf_fx, &fx_sw_user);
-		if (unlikely(ret))
-			return ret;
+		if (check_xstate_in_sigframe(buf_fx, &fx_sw_user))
+			return false;
 
 		fx_only = !fx_sw_user.magic1;
 		state_size = fx_sw_user.xstate_size;
@@ -342,8 +342,8 @@ static int __fpu_restore_sig(void __user
 		 * faults. If it does, fall back to the slow path below, going
 		 * through the kernel buffer with the enabled pagefault handler.
 		 */
-		return restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only,
-						state_size);
+		return !restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only,
+						 state_size);
 	}
 
 	/*
@@ -351,9 +351,8 @@ static int __fpu_restore_sig(void __user
 	 * to be ignored for histerical raisins. The legacy state is folded
 	 * in once the larger state has been copied.
 	 */
-	ret = __copy_from_user(&env, buf, sizeof(env));
-	if (ret)
-		return ret;
+	if (__copy_from_user(&env, buf, sizeof(env)))
+		return false;
 
 	/*
 	 * By setting TIF_NEED_FPU_LOAD it is ensured that our xstate is
@@ -380,17 +379,16 @@ static int __fpu_restore_sig(void __user
 	fpregs_unlock();
 
 	if (use_xsave() && !fx_only) {
-		ret = copy_sigframe_from_user_to_xstate(&fpu->state.xsave, buf_fx);
-		if (ret)
-			return ret;
+		if (copy_sigframe_from_user_to_xstate(&fpu->state.xsave, buf_fx))
+			return false;
 	} else {
 		if (__copy_from_user(&fpu->state.fxsave, buf_fx,
 				     sizeof(fpu->state.fxsave)))
-			return -EFAULT;
+			return false;
 
 		/* Reject invalid MXCSR values. */
 		if (fpu->state.fxsave.mxcsr & ~mxcsr_feature_mask)
-			return -EINVAL;
+			return false;
 
 		/* Enforce XFEATURE_MASK_FPSSE when XSAVE is enabled */
 		if (use_xsave())
@@ -414,17 +412,18 @@ static int __fpu_restore_sig(void __user
 		u64 mask = user_xfeatures | xfeatures_mask_supervisor();
 
 		fpu->state.xsave.header.xfeatures &= mask;
-		ret = os_xrstor_safe(&fpu->state.xsave, xfeatures_mask_all);
+		success = !os_xrstor_safe(&fpu->state.xsave, xfeatures_mask_all);
 	} else {
-		ret = fxrstor_safe(&fpu->state.fxsave);
+		success = !fxrstor_safe(&fpu->state.fxsave);
 	}
 
-	if (likely(!ret))
+	if (likely(success))
 		fpregs_mark_activate();
 
 	fpregs_unlock();
-	return ret;
+	return success;
 }
+
 static inline int xstate_sigframe_size(void)
 {
 	return use_xsave() ? fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE :
@@ -468,7 +467,7 @@ bool fpu__restore_sig(void __user *buf,
 					   sizeof(struct user_i387_ia32_struct),
 					   NULL, buf);
 	} else {
-		success = !__fpu_restore_sig(buf, buf_fx, ia32_fxstate);
+		success = __fpu_restore_sig(buf, buf_fx, ia32_fxstate);
 	}
 
 out:

[patch 09/10] x86/fpu/signal: Change return code of check_xstate_in_sigframe() to boolean

[Thomas Gleixner]

__fpu_sig_restore() only needs success/fail information and no detailed
error code.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -23,8 +23,8 @@ static struct _fpx_sw_bytes fx_sw_reserv
  * Check for the presence of extended state information in the
  * user fpstate pointer in the sigcontext.
  */
-static inline int check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
-					   struct _fpx_sw_bytes *fx_sw)
+static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
+					    struct _fpx_sw_bytes *fx_sw)
 {
 	int min_xstate_size = sizeof(struct fxregs_state) +
 			      sizeof(struct xstate_header);
@@ -32,7 +32,7 @@ static inline int check_xstate_in_sigfra
 	unsigned int magic2;
 
 	if (__copy_from_user(fx_sw, &fxbuf->sw_reserved[0], sizeof(*fx_sw)))
-		return -EFAULT;
+		return false;
 
 	/* Check for the first magic field and other error scenarios. */
 	if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
@@ -48,10 +48,10 @@ static inline int check_xstate_in_sigfra
 	 * in the memory layout.
 	 */
 	if (__get_user(magic2, (__u32 __user *)(fpstate + fx_sw->xstate_size)))
-		return -EFAULT;
+		return false;
 
 	if (likely(magic2 == FP_XSTATE_MAGIC2))
-		return 0;
+		return true;
 setfx:
 	trace_x86_fpu_xstate_check_failed(&current->thread.fpu);
 
@@ -59,7 +59,7 @@ static inline int check_xstate_in_sigfra
 	fx_sw->magic1 = 0;
 	fx_sw->xstate_size = sizeof(struct fxregs_state);
 	fx_sw->xfeatures = XFEATURE_MASK_FPSSE;
-	return 0;
+	return true;
 }
 
 /*
@@ -325,7 +325,7 @@ static bool __fpu_restore_sig(void __use
 	if (use_xsave()) {
 		struct _fpx_sw_bytes fx_sw_user;
 
-		if (check_xstate_in_sigframe(buf_fx, &fx_sw_user))
+		if (!check_xstate_in_sigframe(buf_fx, &fx_sw_user))
 			return false;
 
 		fx_only = !fx_sw_user.magic1;

[patch 10/10] x86/fpu/signal: Change return code of restore_fpregs_from_user() to boolean

[Thomas Gleixner]

__fpu_sig_restore() only needs information about success or fail and no
real error code.

This cleans up the confusing conversion from the negated trap number which
is returned by the *RSTOR() exception fixups to an error code.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -255,8 +255,8 @@ static int __restore_fpregs_from_user(vo
  * Attempt to restore the FPU registers directly from user memory.
  * Pagefaults are handled and any errors returned are fatal.
  */
-static int restore_fpregs_from_user(void __user *buf, u64 xrestore,
-				    bool fx_only, unsigned int size)
+static bool restore_fpregs_from_user(void __user *buf, u64 xrestore,
+				     bool fx_only, unsigned int size)
 {
 	struct fpu *fpu = &current->thread.fpu;
 	int ret;
@@ -285,12 +285,11 @@ static int restore_fpregs_from_user(void
 
 		/* Try to handle #PF, but anything else is fatal. */
 		if (ret != -X86_TRAP_PF)
-			return -EINVAL;
+			return false;
 
-		ret = fault_in_pages_readable(buf, size);
-		if (!ret)
+		if (!fault_in_pages_readable(buf, size))
 			goto retry;
-		return ret;
+		return false;
 	}
 
 	/*
@@ -307,7 +306,7 @@ static int restore_fpregs_from_user(void
 
 	fpregs_mark_activate();
 	fpregs_unlock();
-	return 0;
+	return true;
 }
 
 static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
@@ -342,8 +341,8 @@ static bool __fpu_restore_sig(void __use
 		 * faults. If it does, fall back to the slow path below, going
 		 * through the kernel buffer with the enabled pagefault handler.
 		 */
-		return !restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only,
-						 state_size);
+		return restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only,
+						state_size);
 	}
 
 	/*

Re: [patch 00/10] x86/fpu: Clean up error handling in sigframe related code

[Linus Torvalds]

On Mon, Aug 30, 2021 at 9:27 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> The following series cleans this up. As a side effect the resulting text
> size of fpu/signal.o shrinks by about 150 bytes.

Well, some of the patches in the middle were confusing because of how
0/ERROR was mixing with a success true/false thing, but the end result
seems to be a whole lot more sensible.

Thanks,

             Linus

Re: [patch 00/10] x86/fpu: Clean up error handling in sigframe related code

[Thomas Gleixner]

On Mon, Aug 30 2021 at 10:39, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 9:27 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> The following series cleans this up. As a side effect the resulting text
>> size of fpu/signal.o shrinks by about 150 bytes.
>
> Well, some of the patches in the middle were confusing because of how
> 0/ERROR was mixing with a success true/false thing, but the end result
> seems to be a whole lot more sensible.

Yes, I know. I tried various approaches but they mostly ended up being
unreviewable because they changed too many things at once.

So at the end I went for this step wise approach which seemed to be
manageable at least for my restricted brain capacity.

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Borislav Petkov]

On Mon, Aug 30, 2021 at 06:27:22PM +0200, Thomas Gleixner wrote:
> FPU restore from a signal frame can trigger various exceptions. The
> exceptions are caught with an exception table entry. The handler of this
> entry sets the error return value to the negated exception number.
> 
> Any other exception than #PF is fatal and recovery is not possible. This
> relies on the fact that the #PF exception number is the same as EFAULT, but
> that's not really obvious.
> 
> Check the error code for -X86_TRAP_PF instead of checking it for -EFAULT to
> make it clear how that works.

I guess you wanna fixup the comment over XSTATE_OP() too and perhaps
mention ex_handler_fault() explicitly so that one can make her/his way
around the code and pinpoint quickly where it sticks that exception
number into rAX.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Borislav Petkov]

On Mon, Aug 30, 2021 at 09:33:42PM +0200, Borislav Petkov wrote:
> I guess you wanna fixup the comment over XSTATE_OP() too and perhaps
> mention ex_handler_fault() explicitly so that one can make her/his way
> around the code and pinpoint quickly where it sticks that exception
> number into rAX.

Diff ontop. I think it should not say anything about X86_TRAP_PF or
EFAULT or any other error value but simply that it returns one. It is
callers' task to act upon the specific error value returned.

---
diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 5771af87e4b4..d59bc5df7438 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -199,7 +199,7 @@ static inline void fxsave(struct fxregs_state *fx)
 
 /*
  * After this @err contains 0 on success or the negated trap number when
- * the operation raises an exception. For faults this results in -EFAULT.
+ * the operation raises an exception, see ex_handler_fault().
  */
 #define XSTATE_OP(op, st, lmask, hmask, err)				\
 	asm volatile("1:" op "\n\t"					\

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

On Mon, Aug 30 2021 at 21:33, Borislav Petkov wrote:
> On Mon, Aug 30, 2021 at 06:27:22PM +0200, Thomas Gleixner wrote:
>> FPU restore from a signal frame can trigger various exceptions. The
>> exceptions are caught with an exception table entry. The handler of this
>> entry sets the error return value to the negated exception number.
>> 
>> Any other exception than #PF is fatal and recovery is not possible. This
>> relies on the fact that the #PF exception number is the same as EFAULT, but
>> that's not really obvious.
>> 
>> Check the error code for -X86_TRAP_PF instead of checking it for -EFAULT to
>> make it clear how that works.
>
> I guess you wanna fixup the comment over XSTATE_OP() too and perhaps
> mention ex_handler_fault() explicitly so that one can make her/his way
> around the code and pinpoint quickly where it sticks that exception
> number into rAX.

Something like the below?

Thanks,

        tglx
---
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -88,7 +88,11 @@ static inline void fpstate_init_soft(str
 #endif
 extern void save_fpregs_to_fpstate(struct fpu *fpu);
 
-/* Returns 0 or the negated trap number, which results in -EFAULT for #PF */
+/*
+ * Returns 0 on success or the negated trap number when the operation
+ * raises an exception (The exception fixup function ex_handler_fault()
+ * stores the trap number in EAX).
+ */
 #define user_insn(insn, output, input...)				\
 ({									\
 	int err;							\
@@ -199,7 +203,8 @@ static inline void fxsave(struct fxregs_
 
 /*
  * After this @err contains 0 on success or the negated trap number when
- * the operation raises an exception. For faults this results in -EFAULT.
+ * the operation raises an exception (The exception fixup function
+ * ex_handler_fault() stores the trap number in EAX).
  */
 #define XSTATE_OP(op, st, lmask, hmask, err)				\
 	asm volatile("1:" op "\n\t"					\

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Al Viro]

On Mon, Aug 30, 2021 at 10:09:27PM +0200, Thomas Gleixner wrote:

> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -88,7 +88,11 @@ static inline void fpstate_init_soft(str
>  #endif
>  extern void save_fpregs_to_fpstate(struct fpu *fpu);
>  
> -/* Returns 0 or the negated trap number, which results in -EFAULT for #PF */
> +/*
> + * Returns 0 on success or the negated trap number when the operation
> + * raises an exception (The exception fixup function ex_handler_fault()
> + * stores the trap number in EAX).
> + */
>  #define user_insn(insn, output, input...)				\
>  ({									\
>  	int err;							\
> @@ -199,7 +203,8 @@ static inline void fxsave(struct fxregs_
>  
>  /*
>   * After this @err contains 0 on success or the negated trap number when
> - * the operation raises an exception. For faults this results in -EFAULT.
> + * the operation raises an exception (The exception fixup function
> + * ex_handler_fault() stores the trap number in EAX).
>   */
>  #define XSTATE_OP(op, st, lmask, hmask, err)				\
>  	asm volatile("1:" op "\n\t"					\

Incidentally, why do we bother with negation in those?  Why not have
user_insn(), XSTATE_OP() and kernel_insn_err() return 0 or trap number...

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Linus Torvalds]

On Mon, Aug 30, 2021 at 2:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Incidentally, why do we bother with negation in those?  Why not have
> user_insn(), XSTATE_OP() and kernel_insn_err() return 0 or trap number...

I really wish we didn't have that odd _ASM_EXTABLE_FAULT/
ex_handler_fault() special case at all.

It's *very* confusing, and it actually seems to be mis-used. It looks
like the "copy_mc_fragile" code uses it by mistake, and doesn't
actually want that "modify %%rax" behavior of that exception handler
AT ALL.

If I read that code correctly, it almost by mistake doesn't actually
care, and will overwrite %%rax with the right result, but it doesn't
look like the "fault code in %eax" was ever *intentional*. There's no
mention of it.

Maybe I'm misreading that code, but I look at it and just go "Whaa?"

The code in user_insn() clearly *does* use that fault number (and, as
you say, inverts it for some reason), but I wonder how much it really
cares? Could we get rid of it, and just set a fixed error code?

I only checked one user, but that one didn't actually care which fault
it was, it only cared about fault-vs-no-fault.

                Linus

             Linus

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Al Viro]

On Mon, Aug 30, 2021 at 02:26:12PM -0700, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Incidentally, why do we bother with negation in those?  Why not have
> > user_insn(), XSTATE_OP() and kernel_insn_err() return 0 or trap number...
> 
> I really wish we didn't have that odd _ASM_EXTABLE_FAULT/
> ex_handler_fault() special case at all.
> 
> It's *very* confusing, and it actually seems to be mis-used. It looks
> like the "copy_mc_fragile" code uses it by mistake, and doesn't
> actually want that "modify %%rax" behavior of that exception handler
> AT ALL.
> 
> If I read that code correctly, it almost by mistake doesn't actually
> care, and will overwrite %%rax with the right result, but it doesn't
> look like the "fault code in %eax" was ever *intentional*. There's no
> mention of it.
> 
> Maybe I'm misreading that code, but I look at it and just go "Whaa?"
> 
> The code in user_insn() clearly *does* use that fault number (and, as
> you say, inverts it for some reason), but I wonder how much it really
> cares? Could we get rid of it, and just set a fixed error code?
> 
> I only checked one user, but that one didn't actually care which fault
> it was, it only cared about fault-vs-no-fault.

There's a place where we care about #PF vs. #MC (see upthread)...

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Linus Torvalds]

On Mon, Aug 30, 2021 at 2:33 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> There's a place where we care about #PF vs. #MC (see upthread)...

Interestingly (or perhaps not), that case is a problem case in general
for "fault_in_pages_readable()".

That function will only access data every PAGE_SIZE bytes, but if we
have other exceptions that can happen at a cacheline granularity, the
whole "retry after faulting pages in" may fail.

So that kind of

 - try to copy from user space

 - if that fails, do fault_in_pages_readable() and retry

loop can loop forever.

restore_fpregs_from_user() is odd and special in trying to deal with
it by looking at the error code. I'm n ot convinced it's the right
thing to do, since it just means that all the other places we do this
can be problematic.

But since the Intel machine check stuff is so misdesigned and doesn't
work on any normal machines, most people can't test any of this, none
of this matters, and it's only broken on those "serious enterprise
machines" setups that people think are better, but are actually just
almost entirely untested and thus don't work right.

I'm not sure what the right model here is. We might need to make
fault_in_pages_readable() do things a cacheline at a time, at which
point those repeat loops start working, and the error code thing
becomes pointless.

What I _am_ sure about is that the error code model doesn't work. It
may work in that one special case, but that just means that all the
non-special cases are broken.

So I'll argue that it's a fundamentally broken model, and that
_ASM_EXTABLE_FAULT thing is not just confusing, but actively hurtful.

            Linus

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

On Mon, Aug 30 2021 at 14:26, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> Incidentally, why do we bother with negation in those?  Why not have
>> user_insn(), XSTATE_OP() and kernel_insn_err() return 0 or trap
>> number...

Correct.

> I really wish we didn't have that odd _ASM_EXTABLE_FAULT/
> ex_handler_fault() special case at all.
>
> It's *very* confusing, and it actually seems to be mis-used. It looks
> like the "copy_mc_fragile" code uses it by mistake, and doesn't
> actually want that "modify %%rax" behavior of that exception handler
> AT ALL.
>
> If I read that code correctly, it almost by mistake doesn't actually
> care, and will overwrite %%rax with the right result, but it doesn't
> look like the "fault code in %eax" was ever *intentional*. There's no
> mention of it.
>
> Maybe I'm misreading that code, but I look at it and just go "Whaa?"

Ooops. I never looked at that usage site. It indeed does not make use of
that information. The original __mcsafe_copy() made use of it, but that
got removed/replaced long ago.

The other user is SGX which actually uses the trap number in EAX for
failure analysis.

> The code in user_insn() clearly *does* use that fault number (and, as
> you say, inverts it for some reason), but I wonder how much it really
> cares? Could we get rid of it, and just set a fixed error code?
>
> I only checked one user, but that one didn't actually care which fault
> it was, it only cared about fault-vs-no-fault.

The usage sites of user_insn() and XSTATE_OP() need to distinguish:

   - success
   - fail due to #PF (which can be tried to handle)
   - fail due to some other exception (#GP, #MC)

I found that _ASM_EXTABLE_FAULT() mechanism pretty conveniant for this
and the negation was just me being lazy after I discovered that
X86_TRAP_PF == EFAULT. It turned out not to be a brilliant idea, but at
the time it looked great...

So yes, the negation does not matter, but the ability to check whether
the fail was caused by #PF or not matters.

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Linus Torvalds]

On Mon, Aug 30, 2021 at 3:01 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> So yes, the negation does not matter, but the ability to check whether
> the fail was caused by #PF or not matters.

See my secopd email with a note on access granularity, and why this
kind of model doesn't seem to work in general and seems dangerous.

             Linus

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

On Mon, Aug 30 2021 at 15:00, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:33 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> There's a place where we care about #PF vs. #MC (see upthread)...
>
> Interestingly (or perhaps not), that case is a problem case in general
> for "fault_in_pages_readable()".
>
> That function will only access data every PAGE_SIZE bytes, but if we
> have other exceptions that can happen at a cacheline granularity, the
> whole "retry after faulting pages in" may fail.
>
> So that kind of
>
>  - try to copy from user space
>
>  - if that fails, do fault_in_pages_readable() and retry
>
> loop can loop forever.
>
> restore_fpregs_from_user() is odd and special in trying to deal with
> it by looking at the error code. I'm n ot convinced it's the right
> thing to do, since it just means that all the other places we do this
> can be problematic.

It's not only about #MC. *RSTOR can also trigger #GP in case that the
user buffer contains garbage and we clearly don't want to loop forever
on that either.

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Linus Torvalds]

On Mon, Aug 30, 2021 at 3:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> It's not only about #MC. *RSTOR can also trigger #GP in case that the
> user buffer contains garbage and we clearly don't want to loop forever
> on that either.

Ok, that does seem to merit a the error code check.

Ugh. It leaves that #MC case for other random accesses, though.

         Linus

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Al Viro]

On Mon, Aug 30, 2021 at 03:00:06PM -0700, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:33 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > There's a place where we care about #PF vs. #MC (see upthread)...
> 
> Interestingly (or perhaps not), that case is a problem case in general
> for "fault_in_pages_readable()".

s/a/the one and only/

> I'm not sure what the right model here is. We might need to make
> fault_in_pages_readable() do things a cacheline at a time, at which
> point those repeat loops start working, and the error code thing
> becomes pointless.

	We really don't want to do that to fault_in_pages_readable();
a separate primitive doing that - perhaps, but fault_in_pages_readable()
is used on fairly hot paths and all callers except this one don't need
anything of that sort.

	Similar for fault_in_pages_writeable() - there's exactly one
caller that needs the same kind of warranties, only there it's in
arch-independent code and I'm fairly sure that it (btrfs ioctl) really
is broken on arm64...

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

Linus,

On Mon, Aug 30 2021 at 15:00, Linus Torvalds wrote:
> But since the Intel machine check stuff is so misdesigned and doesn't
> work on any normal machines, most people can't test any of this, none
> of this matters, and it's only broken on those "serious enterprise
> machines" setups that people think are better, but are actually just
> almost entirely untested and thus don't work right.

what's worse is that even if you have access to such a machine, there is
no documented way to do proper hardware based error injection.

The injection mechanism which claims to do hardware error injection in
arch/x86/kernel/cpu/mce/inject.c is a farce:

All it does is to "prepare" the MSRs with some fake error values and
raising #MC via int 18 afterwards in the hope that the previously
prepared MSR values are still valid. Great way to test stuff by setting
the MSR to the expected failure value and then raising the exception in
software.

NHM had a documented mechanism to inject at least ECC failures at the
hardware level, but with the later memory controllers this ended up in
the documentation black hole along with all the other undocumented real
HW injection mechanisms which allow actual testing of this stuff.

The HW injection mechanisms definitely exist, but without documentation
they are useless. Intel still thinks that the secrecy around that stuff
is valuable and they can get away with those untestable mechanisms even
for their endeavours in the safety critical space.

It's pretty much the same approach as security through obscurity, but in
the safety case that's even more hillarious.

Though we all know what the 'S' in INTEL stands for... I used to be
Security, but nowadays it's Security _and_ Safety.

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Borislav Petkov]

On Tue, Aug 31, 2021 at 02:34:16AM +0200, Thomas Gleixner wrote:
> what's worse is that even if you have access to such a machine, there is
> no documented way to do proper hardware based error injection.

Oh brother, welcome to my nightmare :) How much time to do you have? We
haz cookies.

> The injection mechanism which claims to do hardware error injection in
> arch/x86/kernel/cpu/mce/inject.c is a farce:

No no, that's an *attempt* to have something which at least works on
the arch level, without having other "agents" involved. Just keep on
readin'...

> All it does is to "prepare" the MSRs with some fake error values and
> raising #MC via int 18 afterwards in the hope that the previously
> prepared MSR values are still valid.

What do you mean? Something might swoop in and overwrite them before the
INT? Bah, we can do some locking but it is not worth it.

> Great way to test stuff by setting the MSR to the expected failure
> value and then raising the exception in software.

No no, the great way to do error injection is the ACPI-spec'ed, firwmare
implemented

drivers/acpi/apei/einj.c

Yap, you heard me right, firmware. And when you hear firmware, you can
imagine how it all works in practice... Yeap, exactly.

We even wrote documentation what to do:

Documentation/firmware-guide/acpi/apei/einj.rst

But but, this is firmware so

- it is f*cking broken in all ways imaginable

- if it works, it doesn't support the error type which you wanna inject

- if it does, enterprise sh*t hw has added value crap which analyzes and
looks at hardware errors first</me rolls eyes, trying to remain serious>
so you might get the error report if you get lucky.

So right now wrt to RAS my approach is: don't let it get worse than it
is. Yap, that's called maintainer resignation.

And all those hw vendors can come at me with the fanciest feature ideas
- my reply is: you wanted to do it all in the BIOS. Go do that there
too.

> NHM had a documented mechanism to inject at least ECC failures at the
> hardware level, but with the later memory controllers this ended up in
> the documentation black hole along with all the other undocumented real
> HW injection mechanisms which allow actual testing of this stuff.
> 
> The HW injection mechanisms definitely exist, but without documentation
> they are useless. Intel still thinks that the secrecy around that stuff
> is valuable and they can get away with those untestable mechanisms even
> for their endeavours in the safety critical space.

My impression with error injection with hw people is just like what they
do with perf counters: it counts *something* right? You should be happy
that it does.

So yeah, hw error injection and RAS in general is a stinking pile of
doodoo. If I knew that then, I would've steered away from it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Luck, Tony]

On Tue, Aug 31, 2021 at 09:39:30AM +0200, Borislav Petkov wrote:
> On Tue, Aug 31, 2021 at 02:34:16AM +0200, Thomas Gleixner wrote:
> No no, the great way to do error injection is the ACPI-spec'ed, firwmare
> implemented
> 
> drivers/acpi/apei/einj.c
> 
> Yap, you heard me right, firmware. And when you hear firmware, you can
> imagine how it all works in practice... Yeap, exactly.

You can imagine all you want. And if your imagination is based
on experiences with very old systems like Haswell (launched in 2015)
then you'd be right to be skeptical of firmware capabilities.

> We even wrote documentation what to do:
> 
> Documentation/firmware-guide/acpi/apei/einj.rst
> 
> But but, this is firmware so
> 
> - it is f*cking broken in all ways imaginable

s/is/was/

> 
> - if it works, it doesn't support the error type which you wanna inject

Memory errors now have very good coverage. Still some issues with PCIe injection.

> - if it does, enterprise sh*t hw has added value crap which analyzes and
> looks at hardware errors first</me rolls eyes, trying to remain serious>
> so you might get the error report if you get lucky.

Turn off eMCA in BIOS to avoid this.

> > The HW injection mechanisms definitely exist, but without documentation
> > they are useless. Intel still thinks that the secrecy around that stuff
> > is valuable and they can get away with those untestable mechanisms even
> > for their endeavours in the safety critical space.

The injection controls in the memory controller can only be accessed
in SMM mode. Some paranoia there that some ring0 attack could inject
errors at random intervals causing major costs to diagnose and replace
"failing" DIMMs. So documentation wouldn't help Linux because it just
can't twiddle the necessary bits in the h/w.

> My impression with error injection with hw people is just like what they
> do with perf counters: it counts *something* right? You should be happy
> that it does.

This was true <= Haswell. But definitely not true now. The h/w groups
now have validation teams that depend on ACPI/EINJ for many of their
system level tests. Those guys are serious about this stuff. While I'll
just inject 1000 errors on a single machine and call it good if it all
goes as expected, those folks have (small) clusters running injection
tests 24x7 for weeks at a time.

Downsides of ACPI/EINJ today:
1) Availability on production machines. It is always disabled by default
in BIOS. OEMs may not provide a setup option to turn it on (or may have
deleted the code to support it completely). Intel's pre-production servers
always have the code, and the setup option to enable.
2) Doesn't inject to 3D-Xpoint (that has its own injection method, but
it is annoying to have to juggle two methods).
3) Hard/impossible to inject into SGX memory (because BIOS is untrusted
and isn't allowed to do a store to push the poison data to DDR).

-Tony

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Borislav Petkov]

On Tue, Aug 31, 2021 at 11:39:21AM -0700, Luck, Tony wrote:
> You can imagine all you want. And if your imagination is based
> on experiences with very old systems like Haswell (launched in 2015)
> then you'd be right to be skeptical of firmware capabilities.

I wish it were only Haswell boxes. Do you remember how many times I've
talked to you in the past about boxes with broken einj? I don't think
they were all Haswell but I haven't kept track.

> Turn off eMCA in BIOS to avoid this.

I'll try. That is, provided there even is such an option.

> The injection controls in the memory controller can only be accessed
> in SMM mode. Some paranoia there that some ring0 attack could inject
> errors at random intervals causing major costs to diagnose and replace
> "failing" DIMMs. So documentation wouldn't help Linux because it just
> can't twiddle the necessary bits in the h/w.

Yah, that's why this thing needs a BIOS switch which controls injection.
And probably they do that already.

> Downsides of ACPI/EINJ today:
> 1) Availability on production machines. It is always disabled by default
> in BIOS.

That's ok.

> OEMs may not provide a setup option to turn it on (or may have deleted
> the code to support it completely).

Yeah, that's practically the same thing I'm complaining about - einj is
just as useless as before in that case.

> Intel's pre-production servers always have the code, and the setup
> option to enable.

Except that only you and a couple of partners have access to such
boxes. I guess tglx has too and if so that at least answers his initial
complaint about not having an injection method to test kernel code.

> 2) Doesn't inject to 3D-Xpoint (that has its own injection method, but
> it is annoying to have to juggle two methods).

I guess that doesn't matter for our use case of wanting to test the MCE
code, provided one can at least inject somewhere.

> 3) Hard/impossible to inject into SGX memory (because BIOS is untrusted
> and isn't allowed to do a store to push the poison data to DDR).

Oh well.

So, I really wanna believe you that injection capability has improved
but until I see it with my own eyes, I will remain very much sceptical.
And considering how firmware and OEMs are at play here, sceptical is
just the right stance.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

On Mon, Aug 30 2021 at 14:26, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> Incidentally, why do we bother with negation in those?  Why not have
>> user_insn(), XSTATE_OP() and kernel_insn_err() return 0 or trap number...
>
> I really wish we didn't have that odd _ASM_EXTABLE_FAULT/
> ex_handler_fault() special case at all.
>
> It's *very* confusing, and it actually seems to be mis-used. It looks
> like the "copy_mc_fragile" code uses it by mistake, and doesn't
> actually want that "modify %%rax" behavior of that exception handler
> AT ALL.
>
> If I read that code correctly, it almost by mistake doesn't actually
> care, and will overwrite %%rax with the right result, but it doesn't
> look like the "fault code in %eax" was ever *intentional*. There's no
> mention of it.
>
> Maybe I'm misreading that code, but I look at it and just go "Whaa?"

It took me a while to figure out why this is actually using that
specific fault handler. And yes, I had a major "Whaa?" moment as well.

ASM_EXTABLE_FAULT() was introduced with commit

  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

blessed by a guy named Torvalds :)

commit 92b0729c34ca ("x86/mm, x86/mce: Add memcpy_mcsafe()") made use of
this and it actually did not use the trap number either:

+       .section .fixup, "ax"
+       /* Return false for any failure */
+.L_memcpy_mcsafe_fail:
+       mov     $1, %rax
+       ret

commit b2f9d678e28c ("x86/mce: Check for faults tagged in
EXTABLE_CLASS_FAULT exception table entries") made use of this in MCE to
allow in kernel recovery. The only thing it uses is checking the
exception handler type.

Bah. I'll fix that up to make that less obscure.

The remaining two use cases (SGX and FPU) make use of the stored trap
number.

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

On Wed, Sep 01 2021 at 14:00, Thomas Gleixner wrote:
>
> commit b2f9d678e28c ("x86/mce: Check for faults tagged in
> EXTABLE_CLASS_FAULT exception table entries") made use of this in MCE to
> allow in kernel recovery. The only thing it uses is checking the
> exception handler type.
>
> Bah. I'll fix that up to make that less obscure.
>
> The remaining two use cases (SGX and FPU) make use of the stored trap
> number.

Though while for the FPU use case we really want to handle the #MC case,
it's not clear to me whether this is actually correct for SGX.

Jarkko, Sean, Dave?

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Sean Christopherson]

On Wed, Sep 01, 2021, Thomas Gleixner wrote:
> On Wed, Sep 01 2021 at 14:00, Thomas Gleixner wrote:
> >
> > commit b2f9d678e28c ("x86/mce: Check for faults tagged in
> > EXTABLE_CLASS_FAULT exception table entries") made use of this in MCE to
> > allow in kernel recovery. The only thing it uses is checking the
> > exception handler type.
> >
> > Bah. I'll fix that up to make that less obscure.
> >
> > The remaining two use cases (SGX and FPU) make use of the stored trap
> > number.
> 
> Though while for the FPU use case we really want to handle the #MC case,
> it's not clear to me whether this is actually correct for SGX.
> 
> Jarkko, Sean, Dave?

Are you asking about #MC specifically, or about SGX consuming the trap number in
general?

For #MC, it's probably a moot point because #MC on ENCLS is not recoverable for
current hardware.  If #MC somehow occurs on ENCLS and doesn't kill the platform,
"handling" the #MC in SGX is probably wrong.  Note, Tony is working on a series to
support recoverable #MC on SGX stuff on future hardware[*], but I'm not sure that's
relevant to this discussion.

As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
!#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
those sanity checks, which have been useful in the past.

For virtualizing SGX, KVM needs the trap number so that it can inject the correct
exception into the guest, e.g. if the guest violates the ENCLS concurrency rules
it should get a #GP, whereas a EPCM violation should #PF.

[*] https://lore.kernel.org/lkml/20210827195543.1667168-1-tony.luck@intel.com

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

Sean,

On Wed, Sep 01 2021 at 16:47, Sean Christopherson wrote:
> On Wed, Sep 01, 2021, Thomas Gleixner wrote:
>> Though while for the FPU use case we really want to handle the #MC case,
>> it's not clear to me whether this is actually correct for SGX.
>> 
>> Jarkko, Sean, Dave?
>
> Are you asking about #MC specifically, or about SGX consuming the trap number in
> general?

#MC

> For #MC, it's probably a moot point because #MC on ENCLS is not recoverable for
> current hardware.  If #MC somehow occurs on ENCLS and doesn't kill the platform,
> "handling" the #MC in SGX is probably wrong.  Note, Tony is working on a series to
> support recoverable #MC on SGX stuff on future hardware[*], but I'm not sure that's
> relevant to this discussion.

Probably not.

> As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
> it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
> guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
> !#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
> those sanity checks, which have been useful in the past.
>
> For virtualizing SGX, KVM needs the trap number so that it can inject the correct
> exception into the guest, e.g. if the guest violates the ENCLS concurrency rules
> it should get a #GP, whereas a EPCM violation should #PF.

Yes, I understood that part, but I was confused about the #MC part.

Thanks for clarifying!

       tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Dave Hansen]

On 9/1/21 9:47 AM, Sean Christopherson wrote:
> As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
> it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
> guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
> !#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
> those sanity checks, which have been useful in the past.

Yeah, for bare-metal SGX, the trap number only determines if we get a
warning or not.  There's no attempt at recovery or any consequential
change in behavior due to the trap number (other than the warning).

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Jarkko Sakkinen]

On Wed, 2021-09-01 at 16:47 +0000, Sean Christopherson wrote:
> On Wed, Sep 01, 2021, Thomas Gleixner wrote:
> > On Wed, Sep 01 2021 at 14:00, Thomas Gleixner wrote:
> > > commit b2f9d678e28c ("x86/mce: Check for faults tagged in
> > > EXTABLE_CLASS_FAULT exception table entries") made use of this in MCE to
> > > allow in kernel recovery. The only thing it uses is checking the
> > > exception handler type.
> > > 
> > > Bah. I'll fix that up to make that less obscure.
> > > 
> > > The remaining two use cases (SGX and FPU) make use of the stored trap
> > > number.
> > 
> > Though while for the FPU use case we really want to handle the #MC case,
> > it's not clear to me whether this is actually correct for SGX.
> > 
> > Jarkko, Sean, Dave?
> 
> Are you asking about #MC specifically, or about SGX consuming the trap number in
> general?
> 
> For #MC, it's probably a moot point because #MC on ENCLS is not recoverable for
> current hardware.  If #MC somehow occurs on ENCLS and doesn't kill the platform,
> "handling" the #MC in SGX is probably wrong.  Note, Tony is working on a series to
> support recoverable #MC on SGX stuff on future hardware[*], but I'm not sure that's
> relevant to this discussion.
> 
> As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
> it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
> guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
> !#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
> those sanity checks, which have been useful in the past.

AFAIK, we do not consider #UD as a bug. Agree with the conclusion that SGX
should never #MC, I just did not get this part. #UD is something that is
useful for SGX run-time.

/Jarkko

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Thomas Gleixner]

On Thu, Sep 02 2021 at 16:08, Jarkko Sakkinen wrote:
> On Wed, 2021-09-01 at 16:47 +0000, Sean Christopherson wrote:
>> As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
>> it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
>> guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
>> !#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
>> those sanity checks, which have been useful in the past.
>
> AFAIK, we do not consider #UD as a bug. Agree with the conclusion that SGX
> should never #MC, I just did not get this part. #UD is something that is
> useful for SGX run-time.

I understood that storing the trap number is useful. I was just
questioning the #MC angle. I.e. pretending that the #MC caused by ENCLS
is recoverable.

Thanks,

        tglx

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Jarkko Sakkinen]

On Thu, 2021-09-02 at 16:08 +0200, Thomas Gleixner wrote:
> On Thu, Sep 02 2021 at 16:08, Jarkko Sakkinen wrote:
> > On Wed, 2021-09-01 at 16:47 +0000, Sean Christopherson wrote:
> > > As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
> > > it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
> > > guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
> > > !#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
> > > those sanity checks, which have been useful in the past.
> > 
> > AFAIK, we do not consider #UD as a bug. Agree with the conclusion that SGX
> > should never #MC, I just did not get this part. #UD is something that is
> > useful for SGX run-time.
> 
> I understood that storing the trap number is useful. I was just
> questioning the #MC angle. I.e. pretending that the #MC caused by ENCLS
> is recoverable.

Absolutely not. 

I mixed up #UD caused by CPU executing inside enclave and ENCLS causing
#UD. Sorry about that.

Because of KVM we have to catch #PF's, given that a new power cycle
in the host resets the state of SGX protected memory in the guest.

> 
> Thanks,
> 
>         tglx

/Jarkko

Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

[Jarkko Sakkinen]

On Fri, 2021-09-03 at 09:00 +0300, Jarkko Sakkinen wrote:
> On Thu, 2021-09-02 at 16:08 +0200, Thomas Gleixner wrote:
> > On Thu, Sep 02 2021 at 16:08, Jarkko Sakkinen wrote:
> > > On Wed, 2021-09-01 at 16:47 +0000, Sean Christopherson wrote:
> > > > As for SGX consuming the trap number in general, it's correct.  For non-KVM usage,
> > > > it's nice to have but not strictly necessary.  Any fault except #PF on ENCLS is
> > > > guaranteed to be a kernel or hardware bug; SGX uses the trap number to WARN on a
> > > > !#PF exception, e.g. on #GP or #UD.  Not having the trap number would mean losing
> > > > those sanity checks, which have been useful in the past.
> > > 
> > > AFAIK, we do not consider #UD as a bug. Agree with the conclusion that SGX
> > > should never #MC, I just did not get this part. #UD is something that is
> > > useful for SGX run-time.
> > 
> > I understood that storing the trap number is useful. I was just
> > questioning the #MC angle. I.e. pretending that the #MC caused by ENCLS
> > is recoverable.
> 
> Absolutely not. 
> 
> I mixed up #UD caused by CPU executing inside enclave and ENCLS causing
> #UD. Sorry about that.
> 
> Because of KVM we have to catch #PF's, given that a new power cycle
> in the host resets the state of SGX protected memory in the guest.

.. catching #PF's makes also quite a lot of sense for the bare
metal case because otherwise we would have to have hook for
power state change that would have invalidate all enclaves
running in the system.

/Jarkko