[ 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/[email protected]/
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
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 <[email protected]>
Reviewed-by: Len Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
The test measures the kernel's signal delivery with different (enough vs.
insufficient) stack sizes.
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Len Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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...)
> On Nov 20, 2020, at 15:04, Jann Horn <[email protected]> wrote:
>
> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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
On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
<[email protected]> wrote:
> > On Nov 20, 2020, at 15:04, Jann Horn <[email protected]> wrote:
> > On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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 &&
> On Nov 24, 2020, at 10:41, Jann Horn <[email protected]> wrote:
>
> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> <[email protected]> wrote:
>>> On Nov 20, 2020, at 15:04, Jann Horn <[email protected]> wrote:
>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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
On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
<[email protected]> wrote:
> > On Nov 24, 2020, at 10:41, Jann Horn <[email protected]> wrote:
> > On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> > <[email protected]> wrote:
> >>> On Nov 20, 2020, at 15:04, Jann Horn <[email protected]> wrote:
> >>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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?
> On Nov 24, 2020, at 12:47, Jann Horn <[email protected]> wrote:
>
> On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
> <[email protected]> wrote:
>>> On Nov 24, 2020, at 10:41, Jann Horn <[email protected]> wrote:
>>> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
>>> <[email protected]> wrote:
>>>>> On Nov 20, 2020, at 15:04, Jann Horn <[email protected]> wrote:
>>>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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
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
On Nov 24, 2020, at 10:41, Jann Horn <[email protected]> wrote:
> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> <[email protected]> wrote:
>>> On Nov 20, 2020, at 15:04, Jann Horn <[email protected]> wrote:
>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <[email protected]> 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