[PATCH v2 0/4] x86: Improve Minimum Alternate Stack Size

[PATCH v2 0/4] x86: Improve Minimum Alternate Stack Size

[Chang S. Bae]

[ We know there are a lot of Intel patches out there this week. We're
  posting this as early as we can in case anyone has bandwidth to take a
  look.  We don't think these are quite ready to be merged, but any review
  is appreciated. ]

During signal entry, the kernel pushes data onto the normal userspace
stack. On x86, the data pushed onto the user stack includes XSAVE state,
which has grown over time as new features and larger registers have been
added to the architecture.

MINSIGSTKSZ is a constant provided in the kernel signal.h headers and
typically distributed in lib-dev(el) packages, e.g. [1]. Its value is
compiled into programs and is part of the user/kernel ABI. The MINSIGSTKSZ
constant indicates to userspace how much data the kernel expects to push on
the user stack, [2][3].

However, this constant is much too small and does not reflect recent
additions to the architecture. For instance, when AVX-512 states are in
use, the signal frame size can be 3.5KB while MINSIGSTKSZ remains 2KB.

The bug report [4] explains this as an ABI issue. The small MINSIGSTKSZ can
cause user stack overflow when delivering a signal.

In this series, we suggest a couple of things:
1. Provide a variable minimum stack size to userspace, as a similar
   approach to [5]
2. Avoid using a too-small alternate stack

Changes from v1 [6]:
* Took stack alignment into account for sigframe size (Dave Martin)

[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/bits/sigstack.h;h=b9dca794da093dc4d41d39db9851d444e1b54d9b;hb=HEAD
[2]: https://www.gnu.org/software/libc/manual/html_node/Signal-Stack.html
[3]: https://man7.org/linux/man-pages/man2/sigaltstack.2.html
[4]: https://bugzilla.kernel.org/show_bug.cgi?id=153531
[5]: https://blog.linuxplumbersconf.org/2017/ocw/system/presentations/4671/original/plumbers-dm-2017.pdf
[6]: https://lore.kernel.org/lkml/20200929205746.6763-1-chang.seok.bae@intel.com/

Chang S. Bae (4):
  x86/signal: Introduce helpers to get the maximum signal frame size
  x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ
  x86/signal: Prevent an alternate stack overflow before a signal
    delivery
  selftest/x86/signal: Include test cases for validating sigaltstack

 arch/x86/ia32/ia32_signal.c               |  11 +-
 arch/x86/include/asm/elf.h                |   4 +
 arch/x86/include/asm/fpu/signal.h         |   2 +
 arch/x86/include/asm/sigframe.h           |  25 +++++
 arch/x86/include/uapi/asm/auxvec.h        |   6 +-
 arch/x86/kernel/cpu/common.c              |   3 +
 arch/x86/kernel/fpu/signal.c              |  20 ++++
 arch/x86/kernel/signal.c                  |  82 +++++++++++++-
 tools/testing/selftests/x86/Makefile      |   2 +-
 tools/testing/selftests/x86/sigaltstack.c | 126 ++++++++++++++++++++++
 10 files changed, 272 insertions(+), 9 deletions(-)
 create mode 100644 tools/testing/selftests/x86/sigaltstack.c

-- 
2.17.1

[PATCH v2 1/4] x86/signal: Introduce helpers to get the maximum signal frame size

[Chang S. Bae]

Signal frames do not have a fixed format and can vary in size when a number
of things change: support XSAVE features, 32 vs. 64-bit apps. Add the code
to support a runtime method for userspace to dynamically discover how large
a signal stack needs to be.

Introduce a new variable, max_frame_size, and helper functions for the
calculation to be used in a new user interface. Set max_frame_size to a
system-wide worst-case value, instead of storing multiple app-specific
values.

Locate the body of the helper function -- fpu__get_fpstate_sigframe_size()
in fpu/signal.c for its relevance.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Acked-by: H.J. Lu <hjl.tools@gmail.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Change from v1:
* Took stack alignment into account for sigframe size (Dave Martin)
---
 arch/x86/include/asm/fpu/signal.h |  2 +
 arch/x86/include/asm/sigframe.h   | 23 ++++++++++++
 arch/x86/kernel/cpu/common.c      |  3 ++
 arch/x86/kernel/fpu/signal.c      | 20 ++++++++++
 arch/x86/kernel/signal.c          | 61 ++++++++++++++++++++++++++++++-
 5 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 7fb516b6893a..5bfbf8f2e5a3 100644
--- a/arch/x86/include/asm/fpu/signal.h
+++ b/arch/x86/include/asm/fpu/signal.h
@@ -29,6 +29,8 @@ unsigned long
 fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
 		     unsigned long *buf_fx, unsigned long *size);
 
+unsigned long fpu__get_fpstate_sigframe_size(void);
+
 extern void fpu__init_prepare_fx_sw_frame(void);
 
 #endif /* _ASM_X86_FPU_SIGNAL_H */
diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
index 84eab2724875..ac77f3f90bc9 100644
--- a/arch/x86/include/asm/sigframe.h
+++ b/arch/x86/include/asm/sigframe.h
@@ -52,6 +52,15 @@ struct rt_sigframe_ia32 {
 	char retcode[8];
 	/* fp state follows here */
 };
+
+#define SIZEOF_sigframe_ia32	sizeof(struct sigframe_ia32)
+#define SIZEOF_rt_sigframe_ia32	sizeof(struct rt_sigframe_ia32)
+
+#else
+
+#define SIZEOF_sigframe_ia32	0
+#define SIZEOF_rt_sigframe_ia32	0
+
 #endif /* defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) */
 
 #ifdef CONFIG_X86_64
@@ -81,8 +90,22 @@ struct rt_sigframe_x32 {
 	/* fp state follows here */
 };
 
+#define SIZEOF_rt_sigframe_x32	sizeof(struct rt_sigframe_x32)
+
 #endif /* CONFIG_X86_X32_ABI */
 
+#define SIZEOF_rt_sigframe	sizeof(struct rt_sigframe)
+
+#else
+
+#define SIZEOF_rt_sigframe	0
+
 #endif /* CONFIG_X86_64 */
 
+#ifndef SIZEOF_rt_sigframe_x32
+#define SIZEOF_rt_sigframe_x32	0
+#endif
+
+void __init init_sigframe_size(void);
+
 #endif /* _ASM_X86_SIGFRAME_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 35ad8480c464..6954932272d5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -58,6 +58,7 @@
 #include <asm/intel-family.h>
 #include <asm/cpu_device_id.h>
 #include <asm/uv/uv.h>
+#include <asm/sigframe.h>
 
 #include "cpu.h"
 
@@ -1331,6 +1332,8 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
 	fpu__init_system(c);
 
+	init_sigframe_size();
+
 #ifdef CONFIG_X86_32
 	/*
 	 * Regardless of whether PCID is enumerated, the SDM says
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index a4ec65317a7f..9f009525f551 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -507,6 +507,26 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
 
 	return sp;
 }
+
+unsigned long fpu__get_fpstate_sigframe_size(void)
+{
+	unsigned long xstate_size = xstate_sigframe_size();
+	unsigned long fsave_header_size = 0;
+
+	/*
+	 * This space is needed on (most) 32-bit kernels, or when a 32-bit
+	 * app is running on a 64-bit kernel. To keep things simple, just
+	 * assume the worst case and always include space for 'freg_state',
+	 * even for 64-bit apps on 64-bit kernels. This wastes a bit of
+	 * space, but keeps the code simple.
+	 */
+	if ((IS_ENABLED(CONFIG_IA32_EMULATION) ||
+	     IS_ENABLED(CONFIG_X86_32)) && use_fxsr())
+		fsave_header_size = sizeof(struct fregs_state);
+
+	return fsave_header_size + xstate_size;
+}
+
 /*
  * Prepare the SW reserved portion of the fxsave memory layout, indicating
  * the presence of the extended state information in the memory layout
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..bae35ff6e744 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -212,6 +212,11 @@ do {									\
  * Set up a signal frame.
  */
 
+/* x86 ABI requires 16-byte alignment */
+#define FRAME_ALIGNMENT	16UL
+
+#define MAX_FRAME_PADDING	(FRAME_ALIGNMENT - 1)
+
 /*
  * Determine which stack to use..
  */
@@ -222,9 +227,9 @@ static unsigned long align_sigframe(unsigned long sp)
 	 * Align the stack pointer according to the i386 ABI,
 	 * i.e. so that on function entry ((sp + 4) & 15) == 0.
 	 */
-	sp = ((sp + 4) & -16ul) - 4;
+	sp = ((sp + 4) & -FRAME_ALIGNMENT) - 4;
 #else /* !CONFIG_X86_32 */
-	sp = round_down(sp, 16) - 8;
+	sp = round_down(sp, FRAME_ALIGNMENT) - 8;
 #endif
 	return sp;
 }
@@ -663,6 +668,58 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	return 0;
 }
 
+/*
+ * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
+ * If a signal frame starts at an unaligned address, extra space is required.
+ * This is the max alignment padding, conservatively.
+ */
+#define MAX_XSAVE_PADDING	63UL
+
+/*
+ * The frame data is composed of the following areas and laid out as:
+ *
+ * -------------------------
+ * | alignment padding     |
+ * -------------------------
+ * | (f)xsave frame        |
+ * -------------------------
+ * | fsave header          |
+ * -------------------------
+ * | alignment padding     |
+ * -------------------------
+ * | siginfo + ucontext    |
+ * -------------------------
+ */
+
+/* max_frame_size tells userspace the worst case signal stack size. */
+static unsigned long __ro_after_init max_frame_size;
+
+void __init init_sigframe_size(void)
+{
+	/*
+	 * Use the largest of possible structure formats. This might
+	 * slightly oversize the frame for 64-bit apps.
+	 */
+
+	if (IS_ENABLED(CONFIG_X86_32) ||
+	    IS_ENABLED(CONFIG_IA32_EMULATION))
+		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
+				     (unsigned long)SIZEOF_rt_sigframe_ia32);
+
+	if (IS_ENABLED(CONFIG_X86_X32_ABI))
+		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
+
+	if (IS_ENABLED(CONFIG_X86_64))
+		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
+
+	max_frame_size += MAX_FRAME_PADDING;
+
+	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;
+
+	/* Userspace expects an aligned size. */
+	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);
+}
+
 static inline int is_ia32_compat_frame(struct ksignal *ksig)
 {
 	return IS_ENABLED(CONFIG_IA32_EMULATION) &&
-- 
2.17.1

[PATCH v2 2/4] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Chang S. Bae]

Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
use by all architectures with sigaltstack(2). Over time, the hardware state
size grew, but these constants did not evolve. Today, literal use of these
constants on several architectures may result in signal stack overflow, and
thus user data corruption.

A few years ago, the ARM team addressed this issue by establishing
getauxval(AT_MINSIGSTKSZ), such that the kernel can supply at runtime value
that is an appropriate replacement on the current and future hardware.

Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
size to userspace via auxv").

Reported-by: Florian Weimer <fweimer@redhat.com>
Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: x86@kernel.org
Cc: libc-alpha@sourceware.org
Cc: linux-arch@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=153531
---
 arch/x86/include/asm/elf.h         | 4 ++++
 arch/x86/include/uapi/asm/auxvec.h | 6 ++++--
 arch/x86/kernel/signal.c           | 5 +++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index b9a5d488f1a5..044b024abea1 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -311,6 +311,7 @@ do {									\
 		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
 	}								\
+	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
 } while (0)
 
 /*
@@ -327,6 +328,7 @@ extern unsigned long task_size_32bit(void);
 extern unsigned long task_size_64bit(int full_addr_space);
 extern unsigned long get_mmap_base(int is_legacy);
 extern bool mmap_address_hint_valid(unsigned long addr, unsigned long len);
+extern unsigned long get_sigframe_size(void);
 
 #ifdef CONFIG_X86_32
 
@@ -348,6 +350,7 @@ do {									\
 	if (vdso64_enabled)						\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR,				\
 			    (unsigned long __force)current->mm->context.vdso); \
+	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
 } while (0)
 
 /* As a historical oddity, the x32 and x86_64 vDSOs are controlled together. */
@@ -356,6 +359,7 @@ do {									\
 	if (vdso64_enabled)						\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR,				\
 			    (unsigned long __force)current->mm->context.vdso); \
+	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
 } while (0)
 
 #define AT_SYSINFO		32
diff --git a/arch/x86/include/uapi/asm/auxvec.h b/arch/x86/include/uapi/asm/auxvec.h
index 580e3c567046..edd7808060e6 100644
--- a/arch/x86/include/uapi/asm/auxvec.h
+++ b/arch/x86/include/uapi/asm/auxvec.h
@@ -10,11 +10,13 @@
 #endif
 #define AT_SYSINFO_EHDR		33
 
+#define AT_MINSIGSTKSZ		51
+
 /* entries in ARCH_DLINFO: */
 #if defined(CONFIG_IA32_EMULATION) || !defined(CONFIG_X86_64)
-# define AT_VECTOR_SIZE_ARCH 2
+# define AT_VECTOR_SIZE_ARCH 3
 #else /* else it's non-compat x86-64 */
-# define AT_VECTOR_SIZE_ARCH 1
+# define AT_VECTOR_SIZE_ARCH 2
 #endif
 
 #endif /* _ASM_X86_AUXVEC_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index bae35ff6e744..ee6f1ceaa7a2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -720,6 +720,11 @@ void __init init_sigframe_size(void)
 	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);
 }
 
+unsigned long get_sigframe_size(void)
+{
+	return max_frame_size;
+}
+
 static inline int is_ia32_compat_frame(struct ksignal *ksig)
 {
 	return IS_ENABLED(CONFIG_IA32_EMULATION) &&
-- 
2.17.1

[PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Chang S. Bae]

The kernel pushes data on the userspace stack when entering a signal. If
using a sigaltstack(), the kernel precisely knows the user stack size.

When the kernel knows that the user stack is too small, avoid the overflow
and do an immediate SIGSEGV instead.

This overflow is known to occur on systems with large XSAVE state. The
effort to increase the size typically used for altstacks reduces the
frequency of these overflows, but this approach is still useful for legacy
binaries.

Here the kernel expects a bit conservative stack size (for 64-bit apps).
Legacy binaries used a too-small sigaltstack would be already overflowed
before this change, if they run on modern hardware.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/ia32/ia32_signal.c     | 11 ++++++++---
 arch/x86/include/asm/sigframe.h |  2 ++
 arch/x86/kernel/signal.c        | 16 +++++++++++++++-
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 81cf22398cd1..85abd9eb79d5 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -210,13 +210,18 @@ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
 	sp = regs->sp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ksig->ka.sa.sa_flags & SA_ONSTACK)
+	if (ksig->ka.sa.sa_flags & SA_ONSTACK) {
+		/* If the altstack might overflow, die with SIGSEGV: */
+		if (!altstack_size_ok(current))
+			return (void __user *)-1L;
+
 		sp = sigsp(sp, ksig);
 	/* This is the legacy signal stack switching. */
-	else if (regs->ss != __USER32_DS &&
+	} else if (regs->ss != __USER32_DS &&
 		!(ksig->ka.sa.sa_flags & SA_RESTORER) &&
-		 ksig->ka.sa.sa_restorer)
+		 ksig->ka.sa.sa_restorer) {
 		sp = (unsigned long) ksig->ka.sa.sa_restorer;
+	}
 
 	sp = fpu__alloc_mathframe(sp, 1, &fx_aligned, &math_size);
 	*fpstate = (struct _fpstate_32 __user *) sp;
diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
index ac77f3f90bc9..c9f2f9ace76f 100644
--- a/arch/x86/include/asm/sigframe.h
+++ b/arch/x86/include/asm/sigframe.h
@@ -106,6 +106,8 @@ struct rt_sigframe_x32 {
 #define SIZEOF_rt_sigframe_x32	0
 #endif
 
+bool altstack_size_ok(struct task_struct *tsk);
+
 void __init init_sigframe_size(void);
 
 #endif /* _ASM_X86_SIGFRAME_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ee6f1ceaa7a2..cee41d684dc2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(sp) == 0)
+		if (sas_ss_flags(sp) == 0) {
+			/* If the altstack might overflow, die with SIGSEGV: */
+			if (!altstack_size_ok(current))
+				return (void __user *)-1L;
+
 			sp = current->sas_ss_sp + current->sas_ss_size;
+		}
 	} else if (IS_ENABLED(CONFIG_X86_32) &&
 		   !onsigstack &&
 		   regs->ss != __USER_DS &&
@@ -725,6 +730,15 @@ unsigned long get_sigframe_size(void)
 	return max_frame_size;
 }
 
+bool altstack_size_ok(struct task_struct *tsk)
+{
+	/*
+	 * Can this task's sigaltstack accommodate the largest
+	 * signal frame the kernel might need?
+	 */
+	return (tsk->sas_ss_size >= max_frame_size);
+}
+
 static inline int is_ia32_compat_frame(struct ksignal *ksig)
 {
 	return IS_ENABLED(CONFIG_IA32_EMULATION) &&
-- 
2.17.1

[PATCH v2 4/4] selftest/x86/signal: Include test cases for validating sigaltstack

[Chang S. Bae]

The test measures the kernel's signal delivery with different (enough vs.
insufficient) stack sizes.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 tools/testing/selftests/x86/Makefile      |   2 +-
 tools/testing/selftests/x86/sigaltstack.c | 126 ++++++++++++++++++++++
 2 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/x86/sigaltstack.c

diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 6703c7906b71..e0c52e5ab49e 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -13,7 +13,7 @@ CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC) trivial_program.c -no-pie)
 TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
 			check_initial_reg_state sigreturn iopl ioperm \
 			test_vdso test_vsyscall mov_ss_trap \
-			syscall_arg_fault fsgsbase_restore
+			syscall_arg_fault fsgsbase_restore sigaltstack
 TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
diff --git a/tools/testing/selftests/x86/sigaltstack.c b/tools/testing/selftests/x86/sigaltstack.c
new file mode 100644
index 000000000000..353679df6901
--- /dev/null
+++ b/tools/testing/selftests/x86/sigaltstack.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define _GNU_SOURCE
+#include <signal.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <string.h>
+#include <err.h>
+#include <errno.h>
+#include <limits.h>
+#include <sys/mman.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+#include <sys/resource.h>
+#include <setjmp.h>
+
+/* sigaltstack()-enforced minimum stack */
+#define ENFORCED_MINSIGSTKSZ	2048
+
+#ifndef AT_MINSIGSTKSZ
+#  define AT_MINSIGSTKSZ	51
+#endif
+
+static int nerrs;
+
+static bool sigalrm_expected;
+
+static unsigned long at_minstack_size;
+
+static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
+		       int flags)
+{
+	struct sigaction sa;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_sigaction = handler;
+	sa.sa_flags = SA_SIGINFO | flags;
+	sigemptyset(&sa.sa_mask);
+	if (sigaction(sig, &sa, 0))
+		err(1, "sigaction");
+}
+
+static void clearhandler(int sig)
+{
+	struct sigaction sa;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_DFL;
+	sigemptyset(&sa.sa_mask);
+	if (sigaction(sig, &sa, 0))
+		err(1, "sigaction");
+}
+
+static int setup_altstack(void *start, unsigned long size)
+{
+	stack_t ss;
+
+	memset(&ss, 0, sizeof(ss));
+	ss.ss_size = size;
+	ss.ss_sp = start;
+
+	return sigaltstack(&ss, NULL);
+}
+
+static jmp_buf jmpbuf;
+
+static void sigsegv(int sig, siginfo_t *info, void *ctx_void)
+{
+	if (sigalrm_expected) {
+		printf("[FAIL]\tSIGSEGV signal delivered.\n");
+		nerrs++;
+	} else {
+		printf("[OK]\tSIGSEGV signal expectedly delivered.\n");
+	}
+
+	siglongjmp(jmpbuf, 1);
+}
+
+static void sigalrm(int sig, siginfo_t *info, void *ctx_void)
+{
+	if (!sigalrm_expected) {
+		printf("[FAIL]\tSIGALRM sigal delivered.\n");
+		nerrs++;
+	} else {
+		printf("[OK]\tSIGALRM signal expectedly delivered.\n");
+	}
+}
+
+static void test_sigaltstack(void *altstack, unsigned long size)
+{
+	if (setup_altstack(altstack, size))
+		err(1, "sigaltstack()");
+
+	sigalrm_expected = (size > at_minstack_size) ? true : false;
+
+	sethandler(SIGSEGV, sigsegv, 0);
+	sethandler(SIGALRM, sigalrm, SA_ONSTACK);
+
+	if (sigsetjmp(jmpbuf, 1) == 0) {
+		printf("[RUN]\tTest an %s sigaltstack\n",
+		       sigalrm_expected ? "enough" : "insufficient");
+		raise(SIGALRM);
+	}
+
+	clearhandler(SIGALRM);
+	clearhandler(SIGSEGV);
+}
+
+int main(void)
+{
+	void *altstack;
+
+	at_minstack_size = getauxval(AT_MINSIGSTKSZ);
+
+	altstack = mmap(NULL, at_minstack_size + SIGSTKSZ, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+	if (altstack == MAP_FAILED)
+		err(1, "mmap()");
+
+	if ((ENFORCED_MINSIGSTKSZ + 1) < at_minstack_size)
+		test_sigaltstack(altstack, ENFORCED_MINSIGSTKSZ + 1);
+
+	test_sigaltstack(altstack, at_minstack_size + SIGSTKSZ);
+
+	return nerrs == 0 ? 0 : 1;
+}
-- 
2.17.1

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Jann Horn]

On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> The kernel pushes data on the userspace stack when entering a signal. If
> using a sigaltstack(), the kernel precisely knows the user stack size.
>
> When the kernel knows that the user stack is too small, avoid the overflow
> and do an immediate SIGSEGV instead.
>
> This overflow is known to occur on systems with large XSAVE state. The
> effort to increase the size typically used for altstacks reduces the
> frequency of these overflows, but this approach is still useful for legacy
> binaries.
>
> Here the kernel expects a bit conservative stack size (for 64-bit apps).
> Legacy binaries used a too-small sigaltstack would be already overflowed
> before this change, if they run on modern hardware.
[...]
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ee6f1ceaa7a2..cee41d684dc2 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>
>         /* This is the X/Open sanctioned signal stack switching.  */
>         if (ka->sa.sa_flags & SA_ONSTACK) {
> -               if (sas_ss_flags(sp) == 0)
> +               if (sas_ss_flags(sp) == 0) {
> +                       /* If the altstack might overflow, die with SIGSEGV: */
> +                       if (!altstack_size_ok(current))
> +                               return (void __user *)-1L;
> +
>                         sp = current->sas_ss_sp + current->sas_ss_size;
> +               }

A couple lines further down, we have this (since commit 14fc9fbc700d):

        /*
         * If we are on the alternate signal stack and would overflow it, don't.
         * Return an always-bogus address instead so we will die with SIGSEGV.
         */
        if (onsigstack && !likely(on_sig_stack(sp)))
                return (void __user *)-1L;

Is that not working?


(It won't handle the case where the kernel fills up almost all of the
alternate stack, and the userspace signal handler then overflows out
of the alternate signal stack. But there isn't much the kernel can do
about that...)

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Bae, Chang Seok]

> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
> 
> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>> 
>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>> index ee6f1ceaa7a2..cee41d684dc2 100644
>> --- a/arch/x86/kernel/signal.c
>> +++ b/arch/x86/kernel/signal.c
>> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>> 
>>        /* This is the X/Open sanctioned signal stack switching.  */
>>        if (ka->sa.sa_flags & SA_ONSTACK) {
>> -               if (sas_ss_flags(sp) == 0)
>> +               if (sas_ss_flags(sp) == 0) {
>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>> +                       if (!altstack_size_ok(current))
>> +                               return (void __user *)-1L;
>> +
>>                        sp = current->sas_ss_sp + current->sas_ss_size;
>> +               }
> 
> A couple lines further down, we have this (since commit 14fc9fbc700d):
> 
>        /*
>         * If we are on the alternate signal stack and would overflow it, don't.
>         * Return an always-bogus address instead so we will die with SIGSEGV.
>         */
>        if (onsigstack && !likely(on_sig_stack(sp)))
>                return (void __user *)-1L;
> 
> Is that not working?

onsigstack is set at the beginning here. If a signal hits under normal stack,
this flag is not set. Then it will miss the overflow.

The added check allows to detect the sigaltstack overflow (always).

Thanks,
Chang

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Jann Horn]

On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
<chang.seok.bae@intel.com> wrote:
> > On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
> > On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> >>
> >> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> >> index ee6f1ceaa7a2..cee41d684dc2 100644
> >> --- a/arch/x86/kernel/signal.c
> >> +++ b/arch/x86/kernel/signal.c
> >> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
> >>
> >>        /* This is the X/Open sanctioned signal stack switching.  */
> >>        if (ka->sa.sa_flags & SA_ONSTACK) {
> >> -               if (sas_ss_flags(sp) == 0)
> >> +               if (sas_ss_flags(sp) == 0) {
> >> +                       /* If the altstack might overflow, die with SIGSEGV: */
> >> +                       if (!altstack_size_ok(current))
> >> +                               return (void __user *)-1L;
> >> +
> >>                        sp = current->sas_ss_sp + current->sas_ss_size;
> >> +               }
> >
> > A couple lines further down, we have this (since commit 14fc9fbc700d):
> >
> >        /*
> >         * If we are on the alternate signal stack and would overflow it, don't.
> >         * Return an always-bogus address instead so we will die with SIGSEGV.
> >         */
> >        if (onsigstack && !likely(on_sig_stack(sp)))
> >                return (void __user *)-1L;
> >
> > Is that not working?
>
> onsigstack is set at the beginning here. If a signal hits under normal stack,
> this flag is not set. Then it will miss the overflow.
>
> The added check allows to detect the sigaltstack overflow (always).

Ah, I think I understand what you're trying to do. But wouldn't the
better approach be to ensure that the existing on_sig_stack() check is
also used if we just switched to the signal stack? Something like:

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..2f57842fb4d6 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct
pt_regs *regs, size_t frame_size,
        unsigned long math_size = 0;
        unsigned long sp = regs->sp;
        unsigned long buf_fx = 0;
-       int onsigstack = on_sig_stack(sp);
+       bool onsigstack = on_sig_stack(sp);
        int ret;

        /* redzone */
@@ -246,8 +246,10 @@ get_sigframe(struct k_sigaction *ka, struct
pt_regs *regs, size_t frame_size,

        /* This is the X/Open sanctioned signal stack switching.  */
        if (ka->sa.sa_flags & SA_ONSTACK) {
-               if (sas_ss_flags(sp) == 0)
+               if (sas_ss_flags(sp) == 0) {
                        sp = current->sas_ss_sp + current->sas_ss_size;
+                       onsigstack = true;
+               }
        } else if (IS_ENABLED(CONFIG_X86_32) &&
                   !onsigstack &&
                   regs->ss != __USER_DS &&

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Bae, Chang Seok]

> On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
> 
> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>>> 
>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>>> index ee6f1ceaa7a2..cee41d684dc2 100644
>>>> --- a/arch/x86/kernel/signal.c
>>>> +++ b/arch/x86/kernel/signal.c
>>>> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>>>> 
>>>>       /* This is the X/Open sanctioned signal stack switching.  */
>>>>       if (ka->sa.sa_flags & SA_ONSTACK) {
>>>> -               if (sas_ss_flags(sp) == 0)
>>>> +               if (sas_ss_flags(sp) == 0) {
>>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>>>> +                       if (!altstack_size_ok(current))
>>>> +                               return (void __user *)-1L;
>>>> +
>>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
>>>> +               }
>>> 
>>> A couple lines further down, we have this (since commit 14fc9fbc700d):
>>> 
>>>       /*
>>>        * If we are on the alternate signal stack and would overflow it, don't.
>>>        * Return an always-bogus address instead so we will die with SIGSEGV.
>>>        */
>>>       if (onsigstack && !likely(on_sig_stack(sp)))
>>>               return (void __user *)-1L;
>>> 
>>> Is that not working?
>> 
>> onsigstack is set at the beginning here. If a signal hits under normal stack,
>> this flag is not set. Then it will miss the overflow.
>> 
>> The added check allows to detect the sigaltstack overflow (always).
> 
> Ah, I think I understand what you're trying to do. But wouldn't the
> better approach be to ensure that the existing on_sig_stack() check is
> also used if we just switched to the signal stack? Something like:
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index be0d7d4152ec..2f57842fb4d6 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct
> pt_regs *regs, size_t frame_size,
>        unsigned long math_size = 0;
>        unsigned long sp = regs->sp;
>        unsigned long buf_fx = 0;
> -       int onsigstack = on_sig_stack(sp);
> +       bool onsigstack = on_sig_stack(sp);
>        int ret;
> 
>        /* redzone */
> @@ -246,8 +246,10 @@ get_sigframe(struct k_sigaction *ka, struct
> pt_regs *regs, size_t frame_size,
> 
>        /* This is the X/Open sanctioned signal stack switching.  */
>        if (ka->sa.sa_flags & SA_ONSTACK) {
> -               if (sas_ss_flags(sp) == 0)
> +               if (sas_ss_flags(sp) == 0) {
>                        sp = current->sas_ss_sp + current->sas_ss_size;
> +                       onsigstack = true;
> +               }
>        } else if (IS_ENABLED(CONFIG_X86_32) &&
>                   !onsigstack &&
>                   regs->ss != __USER_DS &&

Yeah, but wouldn't it better to avoid overwriting user data if we can? The old
check raises segfault *after* overwritten.

The old check is still helpful to detect an overflow from the nested signal(s)
under sigaltstack.

Thanks,
Chang

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Jann Horn]

On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
<chang.seok.bae@intel.com> wrote:
> > On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
> > On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> > <chang.seok.bae@intel.com> wrote:
> >>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
> >>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> >>>>
> >>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> >>>> index ee6f1ceaa7a2..cee41d684dc2 100644
> >>>> --- a/arch/x86/kernel/signal.c
> >>>> +++ b/arch/x86/kernel/signal.c
> >>>> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
> >>>>
> >>>>       /* This is the X/Open sanctioned signal stack switching.  */
> >>>>       if (ka->sa.sa_flags & SA_ONSTACK) {
> >>>> -               if (sas_ss_flags(sp) == 0)
> >>>> +               if (sas_ss_flags(sp) == 0) {
> >>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
> >>>> +                       if (!altstack_size_ok(current))
> >>>> +                               return (void __user *)-1L;
> >>>> +
> >>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
> >>>> +               }
> >>>
> >>> A couple lines further down, we have this (since commit 14fc9fbc700d):
> >>>
> >>>       /*
> >>>        * If we are on the alternate signal stack and would overflow it, don't.
> >>>        * Return an always-bogus address instead so we will die with SIGSEGV.
> >>>        */
> >>>       if (onsigstack && !likely(on_sig_stack(sp)))
> >>>               return (void __user *)-1L;
> >>>
> >>> Is that not working?
> >>
> >> onsigstack is set at the beginning here. If a signal hits under normal stack,
> >> this flag is not set. Then it will miss the overflow.
> >>
> >> The added check allows to detect the sigaltstack overflow (always).
> >
> > Ah, I think I understand what you're trying to do. But wouldn't the
> > better approach be to ensure that the existing on_sig_stack() check is
> > also used if we just switched to the signal stack? Something like:
> >
> > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > index be0d7d4152ec..2f57842fb4d6 100644
> > --- a/arch/x86/kernel/signal.c
> > +++ b/arch/x86/kernel/signal.c
> > @@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct
> > pt_regs *regs, size_t frame_size,
> >        unsigned long math_size = 0;
> >        unsigned long sp = regs->sp;
> >        unsigned long buf_fx = 0;
> > -       int onsigstack = on_sig_stack(sp);
> > +       bool onsigstack = on_sig_stack(sp);
> >        int ret;
> >
> >        /* redzone */
> > @@ -246,8 +246,10 @@ get_sigframe(struct k_sigaction *ka, struct
> > pt_regs *regs, size_t frame_size,
> >
> >        /* This is the X/Open sanctioned signal stack switching.  */
> >        if (ka->sa.sa_flags & SA_ONSTACK) {
> > -               if (sas_ss_flags(sp) == 0)
> > +               if (sas_ss_flags(sp) == 0) {
> >                        sp = current->sas_ss_sp + current->sas_ss_size;
> > +                       onsigstack = true;
> > +               }
> >        } else if (IS_ENABLED(CONFIG_X86_32) &&
> >                   !onsigstack &&
> >                   regs->ss != __USER_DS &&
>
> Yeah, but wouldn't it better to avoid overwriting user data if we can? The old
> check raises segfault *after* overwritten.

Where is that overwrite happening? Between the point where your check
happens, and the point where the old check is, the only calls are to
fpu__alloc_mathframe() and align_sigframe(), right?
fpu__alloc_mathframe() just does some size calculations and doesn't
write anything. align_sigframe() also just does size calculations. Am
I missing something?

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Bae, Chang Seok]

> On Nov 24, 2020, at 12:47, Jann Horn <jannh@google.com> wrote:
> 
> On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
>>> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
>>> <chang.seok.bae@intel.com> wrote:
>>>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>>>>> 
>>>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>>>>> index ee6f1ceaa7a2..cee41d684dc2 100644
>>>>>> --- a/arch/x86/kernel/signal.c
>>>>>> +++ b/arch/x86/kernel/signal.c
>>>>>> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>>>>>> 
>>>>>>      /* This is the X/Open sanctioned signal stack switching.  */
>>>>>>      if (ka->sa.sa_flags & SA_ONSTACK) {
>>>>>> -               if (sas_ss_flags(sp) == 0)
>>>>>> +               if (sas_ss_flags(sp) == 0) {
>>>>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>>>>>> +                       if (!altstack_size_ok(current))
>>>>>> +                               return (void __user *)-1L;
>>>>>> +
>>>>>>                      sp = current->sas_ss_sp + current->sas_ss_size;
>>>>>> +               }
>>>>> 
>>>>> A couple lines further down, we have this (since commit 14fc9fbc700d):
>>>>> 
>>>>>      /*
>>>>>       * If we are on the alternate signal stack and would overflow it, don't.
>>>>>       * Return an always-bogus address instead so we will die with SIGSEGV.
>>>>>       */
>>>>>      if (onsigstack && !likely(on_sig_stack(sp)))
>>>>>              return (void __user *)-1L;
>>>>> 
>>>>> Is that not working?
>>>> 
>>>> onsigstack is set at the beginning here. If a signal hits under normal stack,
>>>> this flag is not set. Then it will miss the overflow.
>>>> 
>>>> The added check allows to detect the sigaltstack overflow (always).
>>> 
>>> Ah, I think I understand what you're trying to do. But wouldn't the
>>> better approach be to ensure that the existing on_sig_stack() check is
>>> also used if we just switched to the signal stack? Something like:
>>> 
>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>> index be0d7d4152ec..2f57842fb4d6 100644
>>> --- a/arch/x86/kernel/signal.c
>>> +++ b/arch/x86/kernel/signal.c
>>> @@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct
>>> pt_regs *regs, size_t frame_size,
>>>       unsigned long math_size = 0;
>>>       unsigned long sp = regs->sp;
>>>       unsigned long buf_fx = 0;
>>> -       int onsigstack = on_sig_stack(sp);
>>> +       bool onsigstack = on_sig_stack(sp);
>>>       int ret;
>>> 
>>>       /* redzone */
>>> @@ -246,8 +246,10 @@ get_sigframe(struct k_sigaction *ka, struct
>>> pt_regs *regs, size_t frame_size,
>>> 
>>>       /* This is the X/Open sanctioned signal stack switching.  */
>>>       if (ka->sa.sa_flags & SA_ONSTACK) {
>>> -               if (sas_ss_flags(sp) == 0)
>>> +               if (sas_ss_flags(sp) == 0) {
>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
>>> +                       onsigstack = true;
>>> +               }
>>>       } else if (IS_ENABLED(CONFIG_X86_32) &&
>>>                  !onsigstack &&
>>>                  regs->ss != __USER_DS &&
>> 
>> Yeah, but wouldn't it better to avoid overwriting user data if we can? The old
>> check raises segfault *after* overwritten.
> 
> Where is that overwrite happening? Between the point where your check
> happens, and the point where the old check is, the only calls are to
> fpu__alloc_mathframe() and align_sigframe(), right?
> fpu__alloc_mathframe() just does some size calculations and doesn't
> write anything. align_sigframe() also just does size calculations. Am
> I missing something?

Yeah, you’re right. Right now, I’m thinking your approach is simpler and
providing almost the same function (unless I’m missing here).

Thanks,
Chang

Re: [PATCH v2 1/4] x86/signal: Introduce helpers to get the maximum signal frame size

[Borislav Petkov]

On Thu, Nov 19, 2020 at 11:02:34AM -0800, Chang S. Bae wrote:
> Signal frames do not have a fixed format and can vary in size when a number
> of things change: support XSAVE features, 32 vs. 64-bit apps. Add the code
> to support a runtime method for userspace to dynamically discover how large
> a signal stack needs to be.
> 
> Introduce a new variable, max_frame_size, and helper functions for the
> calculation to be used in a new user interface. Set max_frame_size to a
> system-wide worst-case value, instead of storing multiple app-specific
> values.
> 
> Locate the body of the helper function -- fpu__get_fpstate_sigframe_size()
> in fpu/signal.c for its relevance.

This sentence is strange and not needed.

> diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
> index 84eab2724875..ac77f3f90bc9 100644
> --- a/arch/x86/include/asm/sigframe.h
> +++ b/arch/x86/include/asm/sigframe.h
> @@ -52,6 +52,15 @@ struct rt_sigframe_ia32 {
>  	char retcode[8];
>  	/* fp state follows here */
>  };
> +
> +#define SIZEOF_sigframe_ia32	sizeof(struct sigframe_ia32)
> +#define SIZEOF_rt_sigframe_ia32	sizeof(struct rt_sigframe_ia32)
> +
> +#else
> +
> +#define SIZEOF_sigframe_ia32	0
> +#define SIZEOF_rt_sigframe_ia32	0
> +
>  #endif /* defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) */
>  
>  #ifdef CONFIG_X86_64
> @@ -81,8 +90,22 @@ struct rt_sigframe_x32 {
>  	/* fp state follows here */
>  };
>  
> +#define SIZEOF_rt_sigframe_x32	sizeof(struct rt_sigframe_x32)
> +
>  #endif /* CONFIG_X86_X32_ABI */
>  
> +#define SIZEOF_rt_sigframe	sizeof(struct rt_sigframe)
> +
> +#else
> +
> +#define SIZEOF_rt_sigframe	0
> +
>  #endif /* CONFIG_X86_64 */
>  
> +#ifndef SIZEOF_rt_sigframe_x32
> +#define SIZEOF_rt_sigframe_x32	0
> +#endif

Those are defined here to be used in only one place -
init_sigframe_size() - where there already is ifdeffery. Just use the
normal sizeof() operator there instead of adding more gunk here.

> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index a4ec65317a7f..9f009525f551 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -507,6 +507,26 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
>  
>  	return sp;
>  }
> +
> +unsigned long fpu__get_fpstate_sigframe_size(void)
> +{
> +	unsigned long xstate_size = xstate_sigframe_size();
> +	unsigned long fsave_header_size = 0;
> +
> +	/*
> +	 * This space is needed on (most) 32-bit kernels, or when a 32-bit
> +	 * app is running on a 64-bit kernel. To keep things simple, just
> +	 * assume the worst case and always include space for 'freg_state',
> +	 * even for 64-bit apps on 64-bit kernels. This wastes a bit of
> +	 * space, but keeps the code simple.
> +	 */
> +	if ((IS_ENABLED(CONFIG_IA32_EMULATION) ||
> +	     IS_ENABLED(CONFIG_X86_32)) && use_fxsr())
> +		fsave_header_size = sizeof(struct fregs_state);
> +
> +	return fsave_header_size + xstate_size;
> +}

I guess this can be simplified to:

unsigned long fpu__get_fpstate_sigframe_size(void)
{
        unsigned long ret = xstate_sigframe_size();

        /*
         * This space is needed on (most) 32-bit kernels, or when a 32-bit
         * app is running on a 64-bit kernel. To keep things simple, just
         * assume the worst case and always include space for 'freg_state',
         * even for 64-bit apps on 64-bit kernels. This wastes a bit of
         * space, but keeps the code simple.
         */
        if ((IS_ENABLED(CONFIG_IA32_EMULATION) ||
             IS_ENABLED(CONFIG_X86_32)) && use_fxsr())
                ret += sizeof(struct fregs_state);

        return ret;
}

Also, this function simply gives you the xstate size, there's no need
for "sigframe" in the name.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 2/4] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Borislav Petkov]

On Thu, Nov 19, 2020 at 11:02:35AM -0800, Chang S. Bae wrote:
> Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
> use by all architectures with sigaltstack(2). Over time, the hardware state
> size grew, but these constants did not evolve. Today, literal use of these
> constants on several architectures may result in signal stack overflow, and
> thus user data corruption.
> 
> A few years ago, the ARM team addressed this issue by establishing
> getauxval(AT_MINSIGSTKSZ), such that the kernel can supply at runtime value
> that is an appropriate replacement on the current and future hardware.
> 
> Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
> added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
> size to userspace via auxv").

I don't see it documented here:

https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man3/getauxval.3

Dunno, now that two architectures will have it, maybe that is good
enough reason to document it.

Adding Michael.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v2 2/4] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Michael Kerrisk (man-pages)]

Hey Dave Marin,

On 11/26/20 6:44 PM, Borislav Petkov wrote:
> On Thu, Nov 19, 2020 at 11:02:35AM -0800, Chang S. Bae wrote:
>> Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
>> use by all architectures with sigaltstack(2). Over time, the hardware state
>> size grew, but these constants did not evolve. Today, literal use of these
>> constants on several architectures may result in signal stack overflow, and
>> thus user data corruption.
>>
>> A few years ago, the ARM team addressed this issue by establishing
>> getauxval(AT_MINSIGSTKSZ), such that the kernel can supply at runtime value
>> that is an appropriate replacement on the current and future hardware.
>>
>> Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
>> added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
>> size to userspace via auxv").
> 
> I don't see it documented here:
> 
> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man3/getauxval.3
> 
> Dunno, now that two architectures will have it, maybe that is good
> enough reason to document it.
> 
> Adding Michael.

Commit 94b07c1f8c39 was your, Dave. Might I convince you to write a 
patch for getauxval(3)?

Thanks,


Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH v2 4/4] selftest/x86/signal: Include test cases for validating sigaltstack

[Borislav Petkov]

On Thu, Nov 19, 2020 at 11:02:37AM -0800, Chang S. Bae wrote:
> +static void test_sigaltstack(void *altstack, unsigned long size)
> +{
> +	if (setup_altstack(altstack, size))
> +		err(1, "sigaltstack()");
> +
> +	sigalrm_expected = (size > at_minstack_size) ? true : false;
> +
> +	sethandler(SIGSEGV, sigsegv, 0);
> +	sethandler(SIGALRM, sigalrm, SA_ONSTACK);
> +
> +	if (sigsetjmp(jmpbuf, 1) == 0) {
> +		printf("[RUN]\tTest an %s sigaltstack\n",

[RUN]   Test an enough sigaltstack

That's not english, pls try again.

[OK]    SIGALRM signal expectedly delivered.

What is "expectedly delivered"?

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v2 1/4] x86/signal: Introduce helpers to get the maximum signal frame size

[Bae, Chang Seok]

> On Nov 25, 2020, at 20:17, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Thu, Nov 19, 2020 at 11:02:34AM -0800, Chang S. Bae wrote:
>> diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
>> index 84eab2724875..ac77f3f90bc9 100644
>> --- a/arch/x86/include/asm/sigframe.h
>> +++ b/arch/x86/include/asm/sigframe.h
>> @@ -52,6 +52,15 @@ struct rt_sigframe_ia32 {
>> 	char retcode[8];
>> 	/* fp state follows here */
>> };
>> +
>> +#define SIZEOF_sigframe_ia32	sizeof(struct sigframe_ia32)
>> +#define SIZEOF_rt_sigframe_ia32	sizeof(struct rt_sigframe_ia32)
>> +
>> +#else
>> +
>> +#define SIZEOF_sigframe_ia32	0
>> +#define SIZEOF_rt_sigframe_ia32	0
>> +
>> #endif /* defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) */
>> 
>> #ifdef CONFIG_X86_64
>> @@ -81,8 +90,22 @@ struct rt_sigframe_x32 {
>> 	/* fp state follows here */
>> };
>> 
>> +#define SIZEOF_rt_sigframe_x32	sizeof(struct rt_sigframe_x32)
>> +
>> #endif /* CONFIG_X86_X32_ABI */
>> 
>> +#define SIZEOF_rt_sigframe	sizeof(struct rt_sigframe)
>> +
>> +#else
>> +
>> +#define SIZEOF_rt_sigframe	0
>> +
>> #endif /* CONFIG_X86_64 */
>> 
>> +#ifndef SIZEOF_rt_sigframe_x32
>> +#define SIZEOF_rt_sigframe_x32	0
>> +#endif
> 
> Those are defined here to be used in only one place -
> init_sigframe_size() - where there already is ifdeffery. Just use the
> normal sizeof() operator there instead of adding more gunk here.

[ Just want to clarify your comment. ]

Admittedly, this is an (ugly) workaround to avoid compile errors.

E.g. when code is written like this in the function:

	if (IS_ENABLED(CONFIG_X86_X32_ABI))
		size = max(size, sizeof(struct rt_sigframe_x32));

and compile with CONFIG_X86_X32_ABI=n, got such a message:

	"invalid application of 'sizeof' to incomplete type 'struct
	sigframe_ia32’"

While the coding-style doc [1] seems to mention this:

   "However, this approach still allows the C compiler to see the
    code inside the block, and check it for correctness (syntax, 
    types, symbol references, etc). Thus, you still have to use an 
    #ifdef if the code inside the block references symbols that 
    will not exist if the condition is not met.”

In general, putting #ifdef in a C file is advised to avoid. I wonder
whether it is okay to include #ifdef in the C file in this case.

Thanks,
Chang

[1] https://www.kernel.org/doc/html/v5.9/process/coding-style.html

Re: [PATCH v2 1/4] x86/signal: Introduce helpers to get the maximum signal frame size

[Borislav Petkov]

On Mon, Nov 30, 2020 at 08:40:32PM +0000, Bae, Chang Seok wrote:
> In general, putting #ifdef in a C file is advised to avoid.

Well, in this case it is arguably better to have the ifdeffery there,
where it is being used.

> I wonder whether it is okay to include #ifdef in the C file in this
> case.

Yes, it's not like this will be the only place where you have #ifdef in
a .c file:

[ ~/kernel/linux> git grep "#ifdef" *.c | wc -l
29401

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

[Bae, Chang Seok]

On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>>> 
>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>>> index ee6f1ceaa7a2..cee41d684dc2 100644
>>>> --- a/arch/x86/kernel/signal.c
>>>> +++ b/arch/x86/kernel/signal.c
>>>> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>>>> 
>>>>       /* This is the X/Open sanctioned signal stack switching.  */
>>>>       if (ka->sa.sa_flags & SA_ONSTACK) {
>>>> -               if (sas_ss_flags(sp) == 0)
>>>> +               if (sas_ss_flags(sp) == 0) {
>>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>>>> +                       if (!altstack_size_ok(current))
>>>> +                               return (void __user *)-1L;
>>>> +
>>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
>>>> +               }
>>> 
>>> A couple lines further down, we have this (since commit 14fc9fbc700d):
>>> 
>>>       /*
>>>        * If we are on the alternate signal stack and would overflow it, don't.
>>>        * Return an always-bogus address instead so we will die with SIGSEGV.
>>>        */
>>>       if (onsigstack && !likely(on_sig_stack(sp)))
>>>               return (void __user *)-1L;
>>> 
>>> Is that not working?
>> 
>> onsigstack is set at the beginning here. If a signal hits under normal stack,
>> this flag is not set. Then it will miss the overflow.
>> 
>> The added check allows to detect the sigaltstack overflow (always).
> 
> Ah, I think I understand what you're trying to do. But wouldn't the
> better approach be to ensure that the existing on_sig_stack() check is
> also used if we just switched to the signal stack? Something like:
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index be0d7d4152ec..2f57842fb4d6 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct
> pt_regs *regs, size_t frame_size,
>        unsigned long math_size = 0;
>        unsigned long sp = regs->sp;
>        unsigned long buf_fx = 0;
> -       int onsigstack = on_sig_stack(sp);
> +       bool onsigstack = on_sig_stack(sp);
>        int ret;
> 
>        /* redzone */
> @@ -246,8 +246,10 @@ get_sigframe(struct k_sigaction *ka, struct
> pt_regs *regs, size_t frame_size,
> 
>        /* This is the X/Open sanctioned signal stack switching.  */
>        if (ka->sa.sa_flags & SA_ONSTACK) {
> -               if (sas_ss_flags(sp) == 0)
> +               if (sas_ss_flags(sp) == 0) {
>                        sp = current->sas_ss_sp + current->sas_ss_size;
> +                       onsigstack = true;

FWIW, here. 

Thanks to the report by Oliver via the kernel test robot, I realized that
this needs to be conditional on the SS_AUTODISARM tag like, :

    onsigstack = !(current->sas_ss_flags & SS_AUTODISARM);

Thanks,
Chang