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 <[email protected]>
Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")
Suggested-by: Jann Horn <[email protected]>
Suggested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Len Brown <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Jann Horn <[email protected]>
Cc: [email protected]
Cc: [email protected]
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
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)
On Apr 22, 2021, at 01:46, David Laight <[email protected]> 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/[email protected]/
[2] https://lore.kernel.org/lkml/CAJvTdKnpWL8y4N_BrCiK7fU0UXERwuuM8o84LUpp7Watxd8STw@mail.gmail.com/
[3] https://lore.kernel.org/lkml/[email protected]/
From: Bae, Chang Seok
> Sent: 22 April 2021 17:31
>
> On Apr 22, 2021, at 01:46, David Laight <[email protected]> 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)
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
On Wed, May 12 2021 at 18:48, Chang Seok Bae wrote:
> On May 11, 2021, at 11:36, Borislav Petkov <[email protected]> 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
On May 11, 2021, at 11:36, Borislav Petkov <[email protected]> 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
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 <[email protected]> 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
On Apr 22, 2021, at 15:04, David Laight <[email protected]> wrote:
> From: Bae, Chang Seok Sent: 22 April 2021 17:31
>>
>> On Apr 22, 2021, at 01:46, David Laight <[email protected]> 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