All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 0/6] Improve Minimum Alternate Stack Size
@ 2021-04-22  4:48 Chang S. Bae
  2021-04-22  4:48   ` Chang S. Bae
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae

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 v7 [12]:
* Improved the overflow check code. (Andy Lutomirski and Borislav Petkov)
* Moved the "Fixes:" tag and the bugzilla link (patch 5 -> patch 3).

Changes from v6 [11]:
* Updated and fixed the documentation. (Borislav Petkov)
* Revised the AT_MINSIGSTKSZ comment. (Borislav Petkov)

Changes form v5 [10]:
* Fixed the overflow detection. (Andy Lutomirski)
* Reverted the AT_MINSIGSTKSZ removal on arm64. (Dave Martin)
* Added a documentation about the x86 AT_MINSIGSTKSZ.
* Supported the existing sigaltstack test to use the new aux vector.

Changes from v4 [9]:
* Moved the aux vector define to the generic header. (Carlos O'Donell)

Changes from v3 [8]:
* Updated the changelog. (Borislav Petkov)
* Revised the test messages again. (Borislav Petkov)

Changes from v2 [7]:
* Simplified the sigaltstack overflow prevention. (Jann Horn)
* Renamed fpstate size helper with cleanup. (Borislav Petkov)
* Cleaned up the signframe struct size defines. (Borislav Petkov)
* Revised the selftest messages. (Borislav Petkov)
* Revised a changelog. (Borislav Petkov)

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/
[7]: https://lore.kernel.org/lkml/20201119190237.626-1-chang.seok.bae@intel.com/
[8]: https://lore.kernel.org/lkml/20201223015312.4882-1-chang.seok.bae@intel.com/
[9]: https://lore.kernel.org/lkml/20210115211038.2072-1-chang.seok.bae@intel.com/
[10]: https://lore.kernel.org/lkml/20210203172242.29644-1-chang.seok.bae@intel.com/
[11]: https://lore.kernel.org/lkml/20210227165911.32757-1-chang.seok.bae@intel.com/
[12]: https://lore.kernel.org/lkml/20210316065215.23768-1-chang.seok.bae@intel.com/

Chang S. Bae (6):
  uapi: Define the aux vector AT_MINSIGSTKSZ
  x86/signal: Introduce helpers to get the maximum signal frame size
  x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ
  selftest/sigaltstack: Use the AT_MINSIGSTKSZ aux vector if available
  x86/signal: Detect and prevent an alternate signal stack overflow
  selftest/x86/signal: Include test cases for validating sigaltstack

 Documentation/x86/elf_auxvec.rst          |  53 +++++++++
 Documentation/x86/index.rst               |   1 +
 arch/x86/include/asm/elf.h                |   4 +
 arch/x86/include/asm/fpu/signal.h         |   2 +
 arch/x86/include/asm/sigframe.h           |   2 +
 arch/x86/include/uapi/asm/auxvec.h        |   4 +-
 arch/x86/kernel/cpu/common.c              |   3 +
 arch/x86/kernel/fpu/signal.c              |  19 ++++
 arch/x86/kernel/signal.c                  |  86 ++++++++++++++-
 include/linux/sched/signal.h              |  19 ++--
 include/uapi/linux/auxvec.h               |   3 +
 tools/testing/selftests/sigaltstack/sas.c |  20 +++-
 tools/testing/selftests/x86/Makefile      |   2 +-
 tools/testing/selftests/x86/sigaltstack.c | 128 ++++++++++++++++++++++
 14 files changed, 325 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/x86/elf_auxvec.rst
 create mode 100644 tools/testing/selftests/x86/sigaltstack.c


base-commit: bf05bf16c76bb44ab5156223e1e58e26dfe30a88
--
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 1/6] uapi: Define the aux vector AT_MINSIGSTKSZ
  2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
@ 2021-04-22  4:48   ` Chang S. Bae
  2021-04-22  4:48 ` [PATCH v8 2/6] x86/signal: Introduce helpers to get the maximum signal frame size Chang S. Bae
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae, linux-arm-kernel

Define the AT_MINSIGSTKSZ in generic Linux. It is already used as generic
ABI in glibc's generic elf.h, and this define will prevent future namespace
conflicts. In particular, x86 is also using this generic definition.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: libc-alpha@sourceware.org
Cc: linux-arch@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
Change from v6:
* Revised the comment. (Borislav Petkov)

Change from v5:
* Reverted the arm64 change. (Dave Martin and Will Deacon)
* Massaged the changelog.

Change from v4:
* Added as a new patch (Carlos O'Donell)
---
 include/uapi/linux/auxvec.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h
index abe5f2b6581b..c7e502bf5a6f 100644
--- a/include/uapi/linux/auxvec.h
+++ b/include/uapi/linux/auxvec.h
@@ -33,5 +33,8 @@
 
 #define AT_EXECFN  31	/* filename of program */
 
+#ifndef AT_MINSIGSTKSZ
+#define AT_MINSIGSTKSZ	51	/* minimal stack size for signal delivery */
+#endif
 
 #endif /* _UAPI_LINUX_AUXVEC_H */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 1/6] uapi: Define the aux vector AT_MINSIGSTKSZ
@ 2021-04-22  4:48   ` Chang S. Bae
  0 siblings, 0 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae, linux-arm-kernel

Define the AT_MINSIGSTKSZ in generic Linux. It is already used as generic
ABI in glibc's generic elf.h, and this define will prevent future namespace
conflicts. In particular, x86 is also using this generic definition.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: libc-alpha@sourceware.org
Cc: linux-arch@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
Change from v6:
* Revised the comment. (Borislav Petkov)

Change from v5:
* Reverted the arm64 change. (Dave Martin and Will Deacon)
* Massaged the changelog.

Change from v4:
* Added as a new patch (Carlos O'Donell)
---
 include/uapi/linux/auxvec.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h
index abe5f2b6581b..c7e502bf5a6f 100644
--- a/include/uapi/linux/auxvec.h
+++ b/include/uapi/linux/auxvec.h
@@ -33,5 +33,8 @@
 
 #define AT_EXECFN  31	/* filename of program */
 
+#ifndef AT_MINSIGSTKSZ
+#define AT_MINSIGSTKSZ	51	/* minimal stack size for signal delivery */
+#endif
 
 #endif /* _UAPI_LINUX_AUXVEC_H */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 2/6] x86/signal: Introduce helpers to get the maximum signal frame size
  2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
  2021-04-22  4:48   ` Chang S. Bae
@ 2021-04-22  4:48 ` Chang S. Bae
  2021-05-03 13:06   ` Borislav Petkov
  2021-04-22  4:48 ` [PATCH v8 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ Chang S. Bae
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.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.

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
---
Changes from v2:
* Renamed the fpstate size helper with cleanup (Borislav Petkov)
* Moved the sigframe struct size defines to where used (Borislav Petkov)
* Removed unneeded sentence in the changelog (Borislav Petkov)

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   |  2 ++
 arch/x86/kernel/cpu/common.c      |  3 ++
 arch/x86/kernel/fpu/signal.c      | 19 +++++++++++
 arch/x86/kernel/signal.c          | 57 +++++++++++++++++++++++++++++--
 5 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 7fb516b6893a..8b6631dffefd 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_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..5b1ed650b124 100644
--- a/arch/x86/include/asm/sigframe.h
+++ b/arch/x86/include/asm/sigframe.h
@@ -85,4 +85,6 @@ struct rt_sigframe_x32 {
 
 #endif /* CONFIG_X86_64 */
 
+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 ab640abe26b6..c49ef3ad34dc 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"
 
@@ -1334,6 +1335,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..dbb304e48f16 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -507,6 +507,25 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
 
 	return sp;
 }
+
+unsigned long fpu__get_fpstate_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;
+}
+
 /*
  * 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 f306e85a08a6..bf1e83d79326 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,54 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	return 0;
 }
 
+/*
+ * There are four different struct types for signal frame: sigframe_ia32,
+ * rt_sigframe_ia32, rt_sigframe_x32, and rt_sigframe. Use the worst case
+ * -- the largest size. It means the size for 64-bit apps is a bit more
+ * than needed, but this keeps the code simple.
+ */
+#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
+# define MAX_FRAME_SIGINFO_UCTXT_SIZE	sizeof(struct sigframe_ia32)
+#else
+# define MAX_FRAME_SIGINFO_UCTXT_SIZE	sizeof(struct rt_sigframe)
+#endif
+
+/*
+ * 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)
+{
+	max_frame_size = MAX_FRAME_SIGINFO_UCTXT_SIZE + MAX_FRAME_PADDING;
+
+	max_frame_size += fpu__get_fpstate_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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ
  2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
  2021-04-22  4:48   ` Chang S. Bae
  2021-04-22  4:48 ` [PATCH v8 2/6] x86/signal: Introduce helpers to get the maximum signal frame size Chang S. Bae
@ 2021-04-22  4:48 ` Chang S. Bae
  2021-04-22  4:48 ` [PATCH v8 4/6] selftest/sigaltstack: Use the AT_MINSIGSTKSZ aux vector if available Chang S. Bae
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae, Fenghua Yu, linux-doc

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). This enables the kernel to 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").

Also, include a documentation to describe x86-specific auxiliary vectors.

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-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from v7:
* Delegated the bugfix notion to the other patch.

Changes from v6:
* Revised the documentation and fixed the build issue. (Borislav Petkov)
* Fixed the vertical alignment of '\'. (Borislav Petkov)

Changes from v5:
* Added a documentation.
---
 Documentation/x86/elf_auxvec.rst   | 53 ++++++++++++++++++++++++++++++
 Documentation/x86/index.rst        |  1 +
 arch/x86/include/asm/elf.h         |  4 +++
 arch/x86/include/uapi/asm/auxvec.h |  4 +--
 arch/x86/kernel/signal.c           |  5 +++
 5 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/x86/elf_auxvec.rst

diff --git a/Documentation/x86/elf_auxvec.rst b/Documentation/x86/elf_auxvec.rst
new file mode 100644
index 000000000000..6c75b26f5efb
--- /dev/null
+++ b/Documentation/x86/elf_auxvec.rst
@@ -0,0 +1,53 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==================================
+x86-specific ELF Auxiliary Vectors
+==================================
+
+This document describes the semantics of the x86 auxiliary vectors.
+
+Introduction
+============
+
+ELF Auxiliary vectors enable the kernel to efficiently provide
+configuration specific parameters to userspace. In this example, a program
+allocates an alternate stack based on the kernel-provided size::
+
+   #include <sys/auxv.h>
+   #include <elf.h>
+   #include <signal.h>
+   #include <stdlib.h>
+   #include <assert.h>
+   #include <err.h>
+
+   #ifndef AT_MINSIGSTKSZ
+   #define AT_MINSIGSTKSZ	51
+   #endif
+
+   ....
+   stack_t ss;
+
+   ss.ss_sp = malloc(ss.ss_size);
+   assert(ss.ss_sp);
+
+   ss.ss_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
+   ss.ss_flags = 0;
+
+   if (sigaltstack(&ss, NULL))
+        err(1, "sigaltstack");
+
+
+The exposed auxiliary vectors
+=============================
+
+AT_SYSINFO is used for locating the vsyscall entry point.  It is not
+exported on 64-bit mode.
+
+AT_SYSINFO_EHDR is the start address of the page containing the vDSO.
+
+AT_MINSIGSTKSZ denotes the minimum stack size required by the kernel to
+deliver a signal to user-space.  AT_MINSIGSTKSZ comprehends the space
+consumed by the kernel to accommodate the user context for the current
+hardware configuration.  It does not comprehend subsequent user-space stack
+consumption, which must be added by the user.  (e.g. Above, user-space adds
+SIGSTKSZ to AT_MINSIGSTKSZ.)
diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
index 4693e192b447..d58614d5cde6 100644
--- a/Documentation/x86/index.rst
+++ b/Documentation/x86/index.rst
@@ -35,3 +35,4 @@ x86-specific Documentation
    sva
    sgx
    features
+   elf_auxvec
diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 9224d40cdefe..18d9b1117871 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -312,6 +312,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)
 
 /*
@@ -328,6 +329,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
 
@@ -349,6 +351,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. */
@@ -357,6 +360,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..6beb55bbefa4 100644
--- a/arch/x86/include/uapi/asm/auxvec.h
+++ b/arch/x86/include/uapi/asm/auxvec.h
@@ -12,9 +12,9 @@
 
 /* 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 bf1e83d79326..ca8fd18fba1f 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -716,6 +716,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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 4/6] selftest/sigaltstack: Use the AT_MINSIGSTKSZ aux vector if available
  2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
                   ` (2 preceding siblings ...)
  2021-04-22  4:48 ` [PATCH v8 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ Chang S. Bae
@ 2021-04-22  4:48 ` Chang S. Bae
  2021-04-22  4:48 ` [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow Chang S. Bae
  2021-04-22  4:48 ` [PATCH v8 6/6] selftest/x86/signal: Include test cases for validating sigaltstack Chang S. Bae
  5 siblings, 0 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae, linux-kselftest

The SIGSTKSZ constant may not represent enough stack size in some
architectures as the hardware state size grows.

Use getauxval(AT_MINSIGSTKSZ) to increase the stack size.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: linux-kselftest@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from v5:
* Added as a new patch.
---
 tools/testing/selftests/sigaltstack/sas.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 8934a3766d20..c53b070755b6 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -17,6 +17,7 @@
 #include <string.h>
 #include <assert.h>
 #include <errno.h>
+#include <sys/auxv.h>
 
 #include "../kselftest.h"
 
@@ -24,6 +25,11 @@
 #define SS_AUTODISARM  (1U << 31)
 #endif
 
+#ifndef AT_MINSIGSTKSZ
+#define AT_MINSIGSTKSZ	51
+#endif
+
+static unsigned int stack_size;
 static void *sstack, *ustack;
 static ucontext_t uc, sc;
 static const char *msg = "[OK]\tStack preserved";
@@ -47,7 +53,7 @@ void my_usr1(int sig, siginfo_t *si, void *u)
 #endif
 
 	if (sp < (unsigned long)sstack ||
-			sp >= (unsigned long)sstack + SIGSTKSZ) {
+			sp >= (unsigned long)sstack + stack_size) {
 		ksft_exit_fail_msg("SP is not on sigaltstack\n");
 	}
 	/* put some data on stack. other sighandler will try to overwrite it */
@@ -108,6 +114,10 @@ int main(void)
 	stack_t stk;
 	int err;
 
+	/* Make sure more than the required minimum. */
+	stack_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
+	ksft_print_msg("[NOTE]\tthe stack size is %lu\n", stack_size);
+
 	ksft_print_header();
 	ksft_set_plan(3);
 
@@ -117,7 +127,7 @@ int main(void)
 	sigaction(SIGUSR1, &act, NULL);
 	act.sa_sigaction = my_usr2;
 	sigaction(SIGUSR2, &act, NULL);
-	sstack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+	sstack = mmap(NULL, stack_size, PROT_READ | PROT_WRITE,
 		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
 	if (sstack == MAP_FAILED) {
 		ksft_exit_fail_msg("mmap() - %s\n", strerror(errno));
@@ -139,7 +149,7 @@ int main(void)
 	}
 
 	stk.ss_sp = sstack;
-	stk.ss_size = SIGSTKSZ;
+	stk.ss_size = stack_size;
 	stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
 	err = sigaltstack(&stk, NULL);
 	if (err) {
@@ -161,7 +171,7 @@ int main(void)
 		}
 	}
 
-	ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+	ustack = mmap(NULL, stack_size, PROT_READ | PROT_WRITE,
 		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
 	if (ustack == MAP_FAILED) {
 		ksft_exit_fail_msg("mmap() - %s\n", strerror(errno));
@@ -170,7 +180,7 @@ int main(void)
 	getcontext(&uc);
 	uc.uc_link = NULL;
 	uc.uc_stack.ss_sp = ustack;
-	uc.uc_stack.ss_size = SIGSTKSZ;
+	uc.uc_stack.ss_size = stack_size;
 	makecontext(&uc, switch_fn, 0);
 	raise(SIGUSR1);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
                   ` (3 preceding siblings ...)
  2021-04-22  4:48 ` [PATCH v8 4/6] selftest/sigaltstack: Use the AT_MINSIGSTKSZ aux vector if available Chang S. Bae
@ 2021-04-22  4:48 ` Chang S. Bae
  2021-04-22  8:46   ` David Laight
  2021-05-11 18:36   ` Borislav Petkov
  2021-04-22  4:48 ` [PATCH v8 6/6] selftest/x86/signal: Include test cases for validating sigaltstack Chang S. Bae
  5 siblings, 2 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae

The kernel pushes context on to the userspace stack to prepare for the
user's signal handler. When the user has supplied an alternate signal
stack, via sigaltstack(2), it is easy for the kernel to verify that the
stack size is sufficient for the current hardware context.

Check if writing the hardware context to the alternate stack will exceed
it's size. If yes, then instead of corrupting user-data and proceeding with
the original signal handler, an immediate SIGSEGV signal is delivered.

Refactor the stack pointer check code from on_sig_stack() and use the new
helper.

While the kernel allows new source code to discover and use a sufficient
alternate signal stack size, this check is still necessary to protect
binaries with insufficient alternate signal stack size from data
corruption.

Reported-by: Florian Weimer <fweimer@redhat.com>
Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")
Suggested-by: Jann Horn <jannh@google.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Jann Horn <jannh@google.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=153531
---
Changes from v7:
* Separated the notion for entering altstack from a nested signal on the
  altstack. (Andy Lutomirski)
* Added the message for sigalstack overflow. (Andy Lutomirski)
* Refactored on_sig_stack(). (Borislav Petkov)
* Included the "Fixes" tag and the bugzilla link as this patch fixes the
  kernel behavior.

Changes from v5:
* Fixed the overflow check. (Andy Lutomirski)
* Updated the changelog.

Changes from v3:
* Updated the changelog (Borislav Petkov)

Changes from v2:
* Simplified the implementation (Jann Horn)
---
 arch/x86/kernel/signal.c     | 24 ++++++++++++++++++++----
 include/linux/sched/signal.h | 19 ++++++++++++-------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ca8fd18fba1f..beec56f845b7 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -239,10 +239,11 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 	     void __user **fpstate)
 {
 	/* Default to using normal stack */
+	bool nested_altstack = on_sig_stack(regs->sp);
+	bool entering_altstack = false;
 	unsigned long math_size = 0;
 	unsigned long sp = regs->sp;
 	unsigned long buf_fx = 0;
-	int onsigstack = on_sig_stack(sp);
 	int ret;
 
 	/* redzone */
@@ -251,15 +252,23 @@ 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)
+		/*
+		 * This checks nested_altstack via sas_ss_flags(). Sensible
+		 * programs use SS_AUTODISARM, which disables that check, and
+		 * programs that don't use SS_AUTODISARM get compatible.
+		 */
+		if (sas_ss_flags(sp) == 0) {
 			sp = current->sas_ss_sp + current->sas_ss_size;
+			entering_altstack = true;
+		}
 	} else if (IS_ENABLED(CONFIG_X86_32) &&
-		   !onsigstack &&
+		   !nested_altstack &&
 		   regs->ss != __USER_DS &&
 		   !(ka->sa.sa_flags & SA_RESTORER) &&
 		   ka->sa.sa_restorer) {
 		/* This is the legacy signal stack switching. */
 		sp = (unsigned long) ka->sa.sa_restorer;
+		entering_altstack = true;
 	}
 
 	sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
@@ -272,8 +281,15 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 	 * 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)))
+	if (unlikely((nested_altstack || entering_altstack) &&
+		     !__on_sig_stack(sp))) {
+
+		if (show_unhandled_signals && printk_ratelimit())
+			pr_info("%s[%d] overflowed sigaltstack",
+				current->comm, task_pid_nr(current));
+
 		return (void __user *)-1L;
+	}
 
 	/* save i387 and extended state */
 	ret = copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size);
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 3f6a0fcaa10c..ae60f838ebb9 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
 #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
 #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
 
+static inline int __on_sig_stack(unsigned long sp)
+{
+#ifdef CONFIG_STACK_GROWSUP
+	return sp >= current->sas_ss_sp &&
+		sp - current->sas_ss_sp < current->sas_ss_size;
+#else
+	return sp > current->sas_ss_sp &&
+		sp - current->sas_ss_sp <= current->sas_ss_size;
+#endif
+}
+
 /*
  * True if we are on the alternate signal stack.
  */
@@ -554,13 +565,7 @@ static inline int on_sig_stack(unsigned long sp)
 	if (current->sas_ss_flags & SS_AUTODISARM)
 		return 0;
 
-#ifdef CONFIG_STACK_GROWSUP
-	return sp >= current->sas_ss_sp &&
-		sp - current->sas_ss_sp < current->sas_ss_size;
-#else
-	return sp > current->sas_ss_sp &&
-		sp - current->sas_ss_sp <= current->sas_ss_size;
-#endif
+	return __on_sig_stack(sp);
 }
 
 static inline int sas_ss_flags(unsigned long sp)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v8 6/6] selftest/x86/signal: Include test cases for validating sigaltstack
  2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
                   ` (4 preceding siblings ...)
  2021-04-22  4:48 ` [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow Chang S. Bae
@ 2021-04-22  4:48 ` Chang S. Bae
  5 siblings, 0 replies; 17+ messages in thread
From: Chang S. Bae @ 2021-04-22  4:48 UTC (permalink / raw)
  To: bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel, chang.seok.bae, linux-kselftest

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
---
Changes from v3:
* Revised test messages again (Borislav Petkov)

Changes from v2:
* Revised test messages (Borislav Petkov)
---
 tools/testing/selftests/x86/Makefile      |   2 +-
 tools/testing/selftests/x86/sigaltstack.c | 128 ++++++++++++++++++++++
 2 files changed, 129 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 333980375bc7..65bba2ae86ee 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_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..f689af75e979
--- /dev/null
+++ b/tools/testing/selftests/x86/sigaltstack.c
@@ -0,0 +1,128 @@
+// 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]\tWrong signal delivered: SIGSEGV (expected SIGALRM).");
+		nerrs++;
+	} else {
+		printf("[OK]\tSIGSEGV signal delivered.\n");
+	}
+
+	siglongjmp(jmpbuf, 1);
+}
+
+static void sigalrm(int sig, siginfo_t *info, void *ctx_void)
+{
+	if (!sigalrm_expected) {
+		printf("[FAIL]\tWrong signal delivered: SIGALRM (expected SIGSEGV).");
+		nerrs++;
+	} else {
+		printf("[OK]\tSIGALRM signal 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)) {
+		printf("[RUN]\tTest an alternate signal stack of %ssufficient size.\n",
+		       sigalrm_expected ? "" : "in");
+		printf("\tRaise SIGALRM. %s is expected to be delivered.\n",
+		       sigalrm_expected ? "It" : "SIGSEGV");
+		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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-04-22  4:48 ` [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow Chang S. Bae
@ 2021-04-22  8:46   ` David Laight
  2021-04-22 16:31     ` Bae, Chang Seok
  2021-05-11 18:36   ` Borislav Petkov
  1 sibling, 1 reply; 17+ messages in thread
From: David Laight @ 2021-04-22  8:46 UTC (permalink / raw)
  To: 'Chang S. Bae', bp, tglx, mingo, luto, x86
  Cc: len.brown, dave.hansen, hjl.tools, Dave.Martin, jannh, mpe,
	carlos, tony.luck, ravi.v.shankar, libc-alpha, linux-arch,
	linux-api, linux-kernel

From: Chang S. Bae
> Sent: 22 April 2021 05:49
> 
> The kernel pushes context on to the userspace stack to prepare for the
> user's signal handler. When the user has supplied an alternate signal
> stack, via sigaltstack(2), it is easy for the kernel to verify that the
> stack size is sufficient for the current hardware context.
> 
> Check if writing the hardware context to the alternate stack will exceed
> it's size. If yes, then instead of corrupting user-data and proceeding with
> the original signal handler, an immediate SIGSEGV signal is delivered.

What happens if SIGSEGV is caught?

> Refactor the stack pointer check code from on_sig_stack() and use the new
> helper.
> 
> While the kernel allows new source code to discover and use a sufficient
> alternate signal stack size, this check is still necessary to protect
> binaries with insufficient alternate signal stack size from data
> corruption.
...
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index 3f6a0fcaa10c..ae60f838ebb9 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
>  #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
>  #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
> 
> +static inline int __on_sig_stack(unsigned long sp)
> +{
> +#ifdef CONFIG_STACK_GROWSUP
> +	return sp >= current->sas_ss_sp &&
> +		sp - current->sas_ss_sp < current->sas_ss_size;
> +#else
> +	return sp > current->sas_ss_sp &&
> +		sp - current->sas_ss_sp <= current->sas_ss_size;
> +#endif
> +}
> +

Those don't look different enough.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-04-22  8:46   ` David Laight
@ 2021-04-22 16:31     ` Bae, Chang Seok
  2021-04-22 22:04       ` David Laight
  0 siblings, 1 reply; 17+ messages in thread
From: Bae, Chang Seok @ 2021-04-22 16:31 UTC (permalink / raw)
  To: David Laight
  Cc: bp, tglx, mingo, luto, x86, Brown, Len, Hansen, Dave, hjl.tools,
	Dave.Martin, jannh, mpe, carlos, Luck, Tony, Shankar, Ravi V,
	libc-alpha, linux-arch, linux-api, linux-kernel

On Apr 22, 2021, at 01:46, David Laight <David.Laight@ACULAB.COM> wrote:
> From: Chang S. Bae
>> Sent: 22 April 2021 05:49
>> 
>> The kernel pushes context on to the userspace stack to prepare for the
>> user's signal handler. When the user has supplied an alternate signal
>> stack, via sigaltstack(2), it is easy for the kernel to verify that the
>> stack size is sufficient for the current hardware context.
>> 
>> Check if writing the hardware context to the alternate stack will exceed
>> it's size. If yes, then instead of corrupting user-data and proceeding with
>> the original signal handler, an immediate SIGSEGV signal is delivered.
> 
> What happens if SIGSEGV is caught?

Boris pointed out the relevant notes before [1]. I think "unpredictable
results" is a somewhat vague statement but process termination is unavoidable
in this situation.

In the thread [1], a new signal number was discussed for the signal delivery
failure, but my takeaway is this SIGSEGV is still recognizable.

FWIW, Len summarized other possible approaches as well [2].

>> Refactor the stack pointer check code from on_sig_stack() and use the new
>> helper.
>> 
>> While the kernel allows new source code to discover and use a sufficient
>> alternate signal stack size, this check is still necessary to protect
>> binaries with insufficient alternate signal stack size from data
>> corruption.
> ...
>> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
>> index 3f6a0fcaa10c..ae60f838ebb9 100644
>> --- a/include/linux/sched/signal.h
>> +++ b/include/linux/sched/signal.h
>> @@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
>> #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
>> #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
>> 
>> +static inline int __on_sig_stack(unsigned long sp)
>> +{
>> +#ifdef CONFIG_STACK_GROWSUP
>> +	return sp >= current->sas_ss_sp &&
>> +		sp - current->sas_ss_sp < current->sas_ss_size;
>> +#else
>> +	return sp > current->sas_ss_sp &&
>> +		sp - current->sas_ss_sp <= current->sas_ss_size;
>> +#endif
>> +}
>> +
> 
> Those don't look different enough.

The difference is on the SS_AUTODISARM flag check.  This refactoring was
suggested as on_sig_stack() brought confusion [3].

Thanks,
Chang

[1] https://lore.kernel.org/lkml/20210414120608.GE10709@zn.tnic/
[2] https://lore.kernel.org/lkml/CAJvTdKnpWL8y4N_BrCiK7fU0UXERwuuM8o84LUpp7Watxd8STw@mail.gmail.com/
[3] https://lore.kernel.org/lkml/20210325212733.GC32296@zn.tnic/





^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-04-22 16:31     ` Bae, Chang Seok
@ 2021-04-22 22:04       ` David Laight
  2021-05-18 20:53         ` Bae, Chang Seok
  0 siblings, 1 reply; 17+ messages in thread
From: David Laight @ 2021-04-22 22:04 UTC (permalink / raw)
  To: 'Bae, Chang Seok'
  Cc: bp, tglx, mingo, luto, x86, Brown, Len, Hansen, Dave, hjl.tools,
	Dave.Martin, jannh, mpe, carlos, Luck, Tony, Shankar, Ravi V,
	libc-alpha, linux-arch, linux-api, linux-kernel

From: Bae, Chang Seok
> Sent: 22 April 2021 17:31

> 
> On Apr 22, 2021, at 01:46, David Laight <David.Laight@ACULAB.COM> wrote:
> > From: Chang S. Bae
> >> Sent: 22 April 2021 05:49
> >>
> >> The kernel pushes context on to the userspace stack to prepare for the
> >> user's signal handler. When the user has supplied an alternate signal
> >> stack, via sigaltstack(2), it is easy for the kernel to verify that the
> >> stack size is sufficient for the current hardware context.
> >>
> >> Check if writing the hardware context to the alternate stack will exceed
> >> it's size. If yes, then instead of corrupting user-data and proceeding with
> >> the original signal handler, an immediate SIGSEGV signal is delivered.
> >
> > What happens if SIGSEGV is caught?
> 
> Boris pointed out the relevant notes before [1]. I think "unpredictable
> results" is a somewhat vague statement but process termination is unavoidable
> in this situation.
> 
> In the thread [1], a new signal number was discussed for the signal delivery
> failure, but my takeaway is this SIGSEGV is still recognizable.
> 
> FWIW, Len summarized other possible approaches as well [2].

Let's see...
I use an on-stack buffer for the alternate stack and then setup
siglongjmp() to return back from whatever ran out of stack space
back to the processing loop.

So my attempts to trap over-deep recursion cause the main stack
to get corrupted - this is a normal stack overwrite that might
be exploitable!

Alternatively I used malloc() and we have a potentially exploitable
heap overrun.

The only thing the kernel can do is to immediately kill the process
(possibly with a core dump).
Since signals can get nested the kernel needs to ensure there
is a reasonably amount of space left after the signal info is
written to the alternate stack.

> 
> >> Refactor the stack pointer check code from on_sig_stack() and use the new
> >> helper.
> >>
> >> While the kernel allows new source code to discover and use a sufficient
> >> alternate signal stack size, this check is still necessary to protect
> >> binaries with insufficient alternate signal stack size from data
> >> corruption.
> > ...
> >> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> >> index 3f6a0fcaa10c..ae60f838ebb9 100644
> >> --- a/include/linux/sched/signal.h
> >> +++ b/include/linux/sched/signal.h
> >> @@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
> >> #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
> >> #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
> >>
> >> +static inline int __on_sig_stack(unsigned long sp)
> >> +{
> >> +#ifdef CONFIG_STACK_GROWSUP
> >> +	return sp >= current->sas_ss_sp &&
> >> +		sp - current->sas_ss_sp < current->sas_ss_size;
> >> +#else
> >> +	return sp > current->sas_ss_sp &&
> >> +		sp - current->sas_ss_sp <= current->sas_ss_size;
> >> +#endif
> >> +}
> >> +
> >
> > Those don't look different enough.
> 
> The difference is on the SS_AUTODISARM flag check.  This refactoring was
> suggested as on_sig_stack() brought confusion [3].

I was just confused by the #ifdef.
Whether %sp points to the last item or the next space is actually
independent of the stack direction.
A stack might usually use pre-decrement and post-increment but it
doesn't have to.
The stack pointer can't be right at one end of the alt-stack
area (because that is the address you'd use when you switch to it),
and if you are any where near the other end you are hosed.
So a common test:
	return (unsigned long)(sp - current->sas_ss_sp) < current->sas_ss_size;
will always work.

It isn't as though the stack pointer should be anywhere else
other than the 'real' thread stack.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 2/6] x86/signal: Introduce helpers to get the maximum signal frame size
  2021-04-22  4:48 ` [PATCH v8 2/6] x86/signal: Introduce helpers to get the maximum signal frame size Chang S. Bae
@ 2021-05-03 13:06   ` Borislav Petkov
  0 siblings, 0 replies; 17+ messages in thread
From: Borislav Petkov @ 2021-05-03 13:06 UTC (permalink / raw)
  To: Chang S. Bae
  Cc: bp, tglx, mingo, luto, x86, len.brown, dave.hansen, hjl.tools,
	Dave.Martin, jannh, mpe, carlos, tony.luck, ravi.v.shankar,
	libc-alpha, linux-arch, linux-api, linux-kernel

On Wed, Apr 21, 2021 at 09:48:52PM -0700, Chang S. Bae wrote:
> +void __init init_sigframe_size(void)
> +{
> +	max_frame_size = MAX_FRAME_SIGINFO_UCTXT_SIZE + MAX_FRAME_PADDING;
> +
> +	max_frame_size += fpu__get_fpstate_size() + MAX_XSAVE_PADDING;
> +
> +	/* Userspace expects an aligned size. */
> +	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);

I guess we want

	pr_info("max sigframe size: %lu\n", max_frame_size);

here so that we can keep an eye on how much this becomes, in practice.

Thx.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-04-22  4:48 ` [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow Chang S. Bae
  2021-04-22  8:46   ` David Laight
@ 2021-05-11 18:36   ` Borislav Petkov
  2021-05-12 18:48     ` Bae, Chang Seok
  1 sibling, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2021-05-11 18:36 UTC (permalink / raw)
  To: Chang S. Bae, Andy Lutomirski
  Cc: bp, tglx, mingo, luto, x86, len.brown, dave.hansen, hjl.tools,
	Dave.Martin, jannh, mpe, carlos, tony.luck, ravi.v.shankar,
	libc-alpha, linux-arch, linux-api, linux-kernel

On Wed, Apr 21, 2021 at 09:48:55PM -0700, Chang S. Bae wrote:
> The kernel pushes context on to the userspace stack to prepare for the
> user's signal handler. When the user has supplied an alternate signal
> stack, via sigaltstack(2), it is easy for the kernel to verify that the
> stack size is sufficient for the current hardware context.
> 
> Check if writing the hardware context to the alternate stack will exceed
> it's size. If yes, then instead of corrupting user-data and proceeding with
> the original signal handler, an immediate SIGSEGV signal is delivered.

So I did play with this more and modified
tools/testing/selftests/sigaltstack/sas.c, see diff at the end. It uses
MINSIGSTKSZ as the alt stack size and with it, sas.c does:

# [NOTE]        the stack size is 2048, AT_MINSIGSTKSZ: 3632
TAP version 13
1..3
ok 1 Initial sigaltstack state was SS_DISABLE
# sstack: 0x7fdc2cbf1000, ss_size: 2048
# [NOTE]        sigaltstack success
# [NOTE]        Will mmap user stack
# [NOTE]        Will getcontext
# [NOTE]        Will makecontext
# [NOTE]        Will raise SIGUSR1
Segmentation fault (core dumped)

and dmesg has:

[ 2245.641230] signal: get_sigframe: nested_altstack: 0, sp: 0x7ffe50a4d9d0, ka->sa.sa_flags: 0xc000004
[ 2245.641240] signal: get_sigframe: SA_ONSTACK, sas_ss_flags(sp): 0x0
[ 2245.641243] signal: get_sigframe: sp: 0x7fdc2cbf1800, entering_altstack
[ 2245.641245] signal: get_sigframe: nested_altstack: 0, entering_altstack: 1, __on_sig_stack: 0

Those are just debugging stuff, ignore them.

[ 2245.641249] signal: sas[8890] overflowed sigaltstack

So we do detect those overflows now.

I clumsily tried to register a SIGSEGV handler with

        act.sa_sigaction = my_sigsegv;
        sigaction(SIGSEGV, &act, NULL);

but that doesn't fire - task gets killed. Maybe I'm doing it wrong.

> @@ -272,8 +281,15 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>  	 * 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)))
> +	if (unlikely((nested_altstack || entering_altstack) &&
> +		     !__on_sig_stack(sp))) {
> +
> +		if (show_unhandled_signals && printk_ratelimit())
> +			pr_info("%s[%d] overflowed sigaltstack",

					This needs a "\n" at the end of the
					string.

> +				current->comm, task_pid_nr(current));
> +
>  		return (void __user *)-1L;
> +	}
>  
>  	/* save i387 and extended state */
>  	ret = copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size);

---
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index c9c254d5791e..19eb9760e0b5 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -234,6 +234,12 @@ static unsigned long align_sigframe(unsigned long sp)
 	return sp;
 }
 
+#define dbg(fmt, args...)					\
+({								\
+	if (!strcmp(current->comm, "sas"))			\
+		 pr_err("%s: " fmt "\n", __func__, ##args);	\
+})
+
 static void __user *
 get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 	     void __user **fpstate)
@@ -250,8 +256,14 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 	if (IS_ENABLED(CONFIG_X86_64))
 		sp -= 128;
 
+	dbg("nested_altstack: %d, sp: 0x%lx, ka->sa.sa_flags: 0x%lx",
+	    nested_altstack, sp, ka->sa.sa_flags);
+
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if (ka->sa.sa_flags & SA_ONSTACK) {
+
+		dbg("SA_ONSTACK, sas_ss_flags(sp): 0x%x", sas_ss_flags(sp));
+
 		/*
 		 * This checks nested_altstack via sas_ss_flags(). Sensible
 		 * programs use SS_AUTODISARM, which disables that check, and
@@ -260,6 +272,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 		if (sas_ss_flags(sp) == 0) {
 			sp = current->sas_ss_sp + current->sas_ss_size;
 			entering_altstack = true;
+			dbg("sp: 0x%lx, entering_altstack", sp);
 		}
 	} else if (IS_ENABLED(CONFIG_X86_32) &&
 		   !nested_altstack &&
@@ -277,6 +290,9 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 
 	sp = align_sigframe(sp - frame_size);
 
+	dbg("nested_altstack: %d, entering_altstack: %d, __on_sig_stack: %d",
+	     nested_altstack, entering_altstack, __on_sig_stack(sp));
+
 	/*
 	 * 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.
diff --git a/tools/testing/selftests/sigaltstack/Makefile b/tools/testing/selftests/sigaltstack/Makefile
index 3e96d5d47036..b5ac8f9f0c7e 100644
--- a/tools/testing/selftests/sigaltstack/Makefile
+++ b/tools/testing/selftests/sigaltstack/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
-CFLAGS = -Wall
+CFLAGS = -Wall -g
 TEST_GEN_PROGS = sas
 
 include ../lib.mk
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index c53b070755b6..f4c4f5418a08 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -39,6 +39,15 @@ struct stk_data {
 	int flag;
 };
 
+static char *my_strcpy(char *dest, const char *src)
+{
+	char *tmp = dest;
+
+	while ((*dest++ = *src++) != '\0')
+		/* nothing */;
+	return tmp;
+}
+
 void my_usr1(int sig, siginfo_t *si, void *u)
 {
 	char *aa;
@@ -60,7 +69,7 @@ void my_usr1(int sig, siginfo_t *si, void *u)
 	aa = alloca(1024);
 	assert(aa);
 	p = (struct stk_data *)(aa + 512);
-	strcpy(p->msg, msg);
+	my_strcpy(p->msg, msg);
 	p->flag = 1;
 	ksft_print_msg("[RUN]\tsignal USR1\n");
 	err = sigaltstack(NULL, &stk);
@@ -101,6 +110,11 @@ void my_usr2(int sig, siginfo_t *si, void *u)
 	}
 }
 
+void my_sigsegv(int sig, siginfo_t *si, void *u)
+{
+	ksft_print_msg("[NOTE]\tsignal SEGV\n");
+}
+
 static void switch_fn(void)
 {
 	ksft_print_msg("[RUN]\tswitched to user ctx\n");
@@ -115,18 +129,33 @@ int main(void)
 	int err;
 
 	/* Make sure more than the required minimum. */
-	stack_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
-	ksft_print_msg("[NOTE]\tthe stack size is %lu\n", stack_size);
+//	stack_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
+	stack_size = MINSIGSTKSZ;
+	ksft_print_msg("[NOTE]\tthe stack size is %lu, AT_MINSIGSTKSZ: %lu\n",
+			stack_size, getauxval(AT_MINSIGSTKSZ));
 
 	ksft_print_header();
 	ksft_set_plan(3);
 
+	/* clear signal set */
 	sigemptyset(&act.sa_mask);
+
+	/* a registered stack_t will be used */
 	act.sa_flags = SA_ONSTACK | SA_SIGINFO;
+
+	/* SA_SIGINFO means sa_sigaction specifies the signal handler */
 	act.sa_sigaction = my_usr1;
+
+	/* change the signal action on SIGUSR1 using @act desc */
 	sigaction(SIGUSR1, &act, NULL);
+
+	/* same for SIGUSR2 */
 	act.sa_sigaction = my_usr2;
 	sigaction(SIGUSR2, &act, NULL);
+
+	act.sa_sigaction = my_sigsegv;
+	sigaction(SIGSEGV, &act, NULL);
+
 	sstack = mmap(NULL, stack_size, PROT_READ | PROT_WRITE,
 		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
 	if (sstack == MAP_FAILED) {
@@ -150,6 +179,8 @@ int main(void)
 
 	stk.ss_sp = sstack;
 	stk.ss_size = stack_size;
+	ksft_print_msg("sstack: 0x%lx, ss_size: %d\n", sstack, stack_size);
+
 	stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
 	err = sigaltstack(&stk, NULL);
 	if (err) {
@@ -169,21 +200,45 @@ int main(void)
 					strerror(errno));
 			return EXIT_FAILURE;
 		}
+	} else {
+		ksft_print_msg("[NOTE]\tsigaltstack success\n");
 	}
 
+	ksft_print_msg("[NOTE]\tWill mmap user stack\n");
+
 	ustack = mmap(NULL, stack_size, PROT_READ | PROT_WRITE,
 		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
 	if (ustack == MAP_FAILED) {
 		ksft_exit_fail_msg("mmap() - %s\n", strerror(errno));
 		return EXIT_FAILURE;
 	}
+
+	ksft_print_msg("[NOTE]\tWill getcontext\n");
+
+	/* init @uc to the currently active context */
 	getcontext(&uc);
+
+	/* do not resume to other context when current context terminates */
 	uc.uc_link = NULL;
+
+	/* set up the stack to use */
 	uc.uc_stack.ss_sp = ustack;
 	uc.uc_stack.ss_size = stack_size;
+
+	ksft_print_msg("[NOTE]\tWill makecontext\n");
+
+	/*
+	 * Run @switch_fn when this context is activated with setcontext or
+	 * swapcontext.
+	 */
 	makecontext(&uc, switch_fn, 0);
+
+	ksft_print_msg("[NOTE]\tWill raise SIGUSR1\n");
+
 	raise(SIGUSR1);
 
+	ksft_print_msg("[NOTE]\tWill sigaltstack\n");
+
 	err = sigaltstack(NULL, &stk);
 	if (err) {
 		ksft_exit_fail_msg("sigaltstack() - %s\n", strerror(errno));


-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-05-11 18:36   ` Borislav Petkov
@ 2021-05-12 18:48     ` Bae, Chang Seok
  2021-05-12 20:55       ` Thomas Gleixner
  0 siblings, 1 reply; 17+ messages in thread
From: Bae, Chang Seok @ 2021-05-12 18:48 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Andy Lutomirski, bp, tglx, mingo, x86, Brown, Len, Hansen, Dave,
	hjl.tools, Dave.Martin, jannh, mpe, carlos, Luck, Tony, Shankar,
	Ravi V, libc-alpha, linux-arch, linux-api, linux-kernel

On May 11, 2021, at 11:36, Borislav Petkov <bp@alien8.de> wrote:
> 
> I clumsily tried to register a SIGSEGV handler with
> 
>        act.sa_sigaction = my_sigsegv;
>        sigaction(SIGSEGV, &act, NULL);
> 
> but that doesn't fire - task gets killed. Maybe I'm doing it wrong.

Since the altstack is already overflowed, perhaps set the flag like this -- not
using it to get the handler:

	act.sa_sigaction = my_sigsegv;
+	act.sa_flags = SA_SIGINFO;
	sigaction(SIGSEGV, &act, NULL);

FWIW, I think this is just a workaround for this case; in practice, altstack is
rather a backup for normal stack corruption.

Thanks,
Chang


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-05-12 18:48     ` Bae, Chang Seok
@ 2021-05-12 20:55       ` Thomas Gleixner
  2021-05-14  7:26         ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-05-12 20:55 UTC (permalink / raw)
  To: Bae, Chang Seok, Borislav Petkov
  Cc: Andy Lutomirski, bp, mingo, x86, Brown, Len, Hansen, Dave,
	hjl.tools, Dave.Martin, jannh, mpe, carlos, Luck, Tony, Shankar,
	Ravi V, libc-alpha, linux-arch, linux-api, linux-kernel

On Wed, May 12 2021 at 18:48, Chang Seok Bae wrote:
> On May 11, 2021, at 11:36, Borislav Petkov <bp@alien8.de> wrote:
>> 
>> I clumsily tried to register a SIGSEGV handler with
>> 
>>        act.sa_sigaction = my_sigsegv;
>>        sigaction(SIGSEGV, &act, NULL);
>> 
>> but that doesn't fire - task gets killed. Maybe I'm doing it wrong.
>
> Since the altstack is already overflowed, perhaps set the flag like this -- not
> using it to get the handler:
>
> 	act.sa_sigaction = my_sigsegv;
> +	act.sa_flags = SA_SIGINFO;
> 	sigaction(SIGSEGV, &act, NULL);
>
> FWIW, I think this is just a workaround for this case; in practice, altstack is
> rather a backup for normal stack corruption.

That's the intended usage, but it's not limited to that and there exists
creative (ab)use of sigaltstack beyond catching the overflow of the
regular stack.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-05-12 20:55       ` Thomas Gleixner
@ 2021-05-14  7:26         ` Borislav Petkov
  0 siblings, 0 replies; 17+ messages in thread
From: Borislav Petkov @ 2021-05-14  7:26 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Bae, Chang Seok, Andy Lutomirski, mingo, x86, Brown, Len, Hansen,
	Dave, hjl.tools, Dave.Martin, jannh, mpe, carlos, Luck, Tony,
	Shankar, Ravi V, libc-alpha, linux-arch, linux-api, linux-kernel

On Wed, May 12, 2021 at 10:55:04PM +0200, Thomas Gleixner wrote:
> On Wed, May 12 2021 at 18:48, Chang Seok Bae wrote:
> > On May 11, 2021, at 11:36, Borislav Petkov <bp@alien8.de> wrote:
> >> 
> >> I clumsily tried to register a SIGSEGV handler with
> >> 
> >>        act.sa_sigaction = my_sigsegv;
> >>        sigaction(SIGSEGV, &act, NULL);
> >> 
> >> but that doesn't fire - task gets killed. Maybe I'm doing it wrong.
> >
> > Since the altstack is already overflowed, perhaps set the flag like this -- not
> > using it to get the handler:
> >
> > 	act.sa_sigaction = my_sigsegv;
> > +	act.sa_flags = SA_SIGINFO;
> > 	sigaction(SIGSEGV, &act, NULL);
> >
> > FWIW, I think this is just a workaround for this case; in practice, altstack is
> > rather a backup for normal stack corruption.
> 
> That's the intended usage, but it's not limited to that and there exists
> creative (ab)use of sigaltstack beyond catching the overflow of the
> regular stack.

Right, with the above sa_flags setting (SA_ONSTACK removed) it does run the
SIGSEGV handler:

# [NOTE]        the stack size is 2048, AT_MINSIGSTKSZ: 3632
TAP version 13
1..3
ok 1 Initial sigaltstack state was SS_DISABLE
# sstack: 0x7f4e2e4d1000, ss_size: 2048
# [NOTE]        sigaltstack success
# [NOTE]        Will mmap user stack
# [NOTE]        Will getcontext
# [NOTE]        Will makecontext
# [NOTE]        Will raise SIGUSR1
# [NOTE]        signal SEGV
^^^^^^^^^^^

# [NOTE]        Will sigaltstack
ok 2 sigaltstack is still SS_AUTODISARM after signal
# Planned tests != run tests (3 != 2)
# Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0

and exits normally. dmesg has:

[220514.661048] signal: get_sigframe: nested_altstack: 0, sp: 0x7ffc2846bca0, ka->sa.sa_flags: 0xc000004
[220514.661058] signal: get_sigframe: SA_ONSTACK, sas_ss_flags(sp): 0x0
[220514.661061] signal: get_sigframe: sp: 0x7f4e2e4d1800, entering_altstack
[220514.661064] signal: get_sigframe: nested_altstack: 0, entering_altstack: 1, __on_sig_stack: 0
[220514.661067] signal: sas[77819] overflowed sigaltstack

so at least we've warned that we've overflowed the sigaltstack.

[220514.661072] signal: get_sigframe: nested_altstack: 0, sp: 0x7ffc2846bca0, ka->sa.sa_flags: 0x4000004
[220514.661075] signal: get_sigframe: nested_altstack: 0, entering_altstack: 0, __on_sig_stack: 0

So I'm not even going to think about claiming that this is taking care
of the other productive ways of (ab)using the sigaltstack contraption
but from where I'm standing, it is not making it worse, AFAICT.

Thx.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
  2021-04-22 22:04       ` David Laight
@ 2021-05-18 20:53         ` Bae, Chang Seok
  0 siblings, 0 replies; 17+ messages in thread
From: Bae, Chang Seok @ 2021-05-18 20:53 UTC (permalink / raw)
  To: David Laight
  Cc: bp, tglx, mingo, luto, x86, Brown, Len, Hansen, Dave, hjl.tools,
	Dave.Martin, jannh, mpe, carlos, Luck, Tony, Shankar, Ravi V,
	libc-alpha, linux-arch, linux-api, linux-kernel

On Apr 22, 2021, at 15:04, David Laight <David.Laight@ACULAB.COM> wrote:
> From: Bae, Chang Seok Sent: 22 April 2021 17:31
>> 
>> On Apr 22, 2021, at 01:46, David Laight <David.Laight@ACULAB.COM> wrote:
>>> From: Chang S. Bae Sent: 22 April 2021 05:49
>>>> 
>>>> 
>>>> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
>>>> index 3f6a0fcaa10c..ae60f838ebb9 100644
>>>> --- a/include/linux/sched/signal.h
>>>> +++ b/include/linux/sched/signal.h
>>>> @@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
>>>> #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
>>>> #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
>>>> 
>>>> +static inline int __on_sig_stack(unsigned long sp)
>>>> +{
>>>> +#ifdef CONFIG_STACK_GROWSUP
>>>> +	return sp >= current->sas_ss_sp &&
>>>> +		sp - current->sas_ss_sp < current->sas_ss_size;
>>>> +#else
>>>> +	return sp > current->sas_ss_sp &&
>>>> +		sp - current->sas_ss_sp <= current->sas_ss_size;
>>>> +#endif
>>>> +}
>>>> +
>>> 
>>> Those don't look different enough.
>> 
>> The difference is on the SS_AUTODISARM flag check.  This refactoring was
>> suggested as on_sig_stack() brought confusion [3].
> 
> I was just confused by the #ifdef.
> Whether %sp points to the last item or the next space is actually
> independent of the stack direction.
> A stack might usually use pre-decrement and post-increment but it
> doesn't have to.
> The stack pointer can't be right at one end of the alt-stack
> area (because that is the address you'd use when you switch to it),
> and if you are any where near the other end you are hosed.
> So a common test:
> 	return (unsigned long)(sp - current->sas_ss_sp) < current->sas_ss_size;
> will always work.
> 
> It isn't as though the stack pointer should be anywhere else
> other than the 'real' thread stack.

Thanks for the suggestion. Yes, this hunk can be made better like that. But I
would make this change as pure refactoring. Perhaps, follow up after this
series.

Chang



^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-05-18 20:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22  4:48 [PATCH v8 0/6] Improve Minimum Alternate Stack Size Chang S. Bae
2021-04-22  4:48 ` [PATCH v8 1/6] uapi: Define the aux vector AT_MINSIGSTKSZ Chang S. Bae
2021-04-22  4:48   ` Chang S. Bae
2021-04-22  4:48 ` [PATCH v8 2/6] x86/signal: Introduce helpers to get the maximum signal frame size Chang S. Bae
2021-05-03 13:06   ` Borislav Petkov
2021-04-22  4:48 ` [PATCH v8 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ Chang S. Bae
2021-04-22  4:48 ` [PATCH v8 4/6] selftest/sigaltstack: Use the AT_MINSIGSTKSZ aux vector if available Chang S. Bae
2021-04-22  4:48 ` [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow Chang S. Bae
2021-04-22  8:46   ` David Laight
2021-04-22 16:31     ` Bae, Chang Seok
2021-04-22 22:04       ` David Laight
2021-05-18 20:53         ` Bae, Chang Seok
2021-05-11 18:36   ` Borislav Petkov
2021-05-12 18:48     ` Bae, Chang Seok
2021-05-12 20:55       ` Thomas Gleixner
2021-05-14  7:26         ` Borislav Petkov
2021-04-22  4:48 ` [PATCH v8 6/6] selftest/x86/signal: Include test cases for validating sigaltstack Chang S. Bae

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.