* [PATCH v2 1/3] MIPS: signal: Protect against sigaltstack wraparound
2021-12-20 4:27 [PATCH v2 0/3] MIPS: signal: Modify some code Tiezhu Yang
@ 2021-12-20 4:27 ` Tiezhu Yang
2022-01-02 13:31 ` Thomas Bogendoerfer
2021-12-20 4:27 ` [PATCH v2 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
2021-12-20 4:27 ` [PATCH v2 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code Tiezhu Yang
2 siblings, 1 reply; 8+ messages in thread
From: Tiezhu Yang @ 2021-12-20 4:27 UTC (permalink / raw)
To: Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel
If a process uses alternative signal stack by using sigaltstack(),
then that stack overflows and stack wraparound occurs.
Simple Explanation:
The accurate sp order is A,B,C,D,...
But now the sp points to A,B,C and A,B,C again.
This problem can reproduce by the following code:
$ cat test_sigaltstack.c
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
volatile int counter = 0;
void print_sp()
{
unsigned long sp;
__asm__ __volatile__("move %0, $sp" : "=r" (sp));
printf("sp = 0x%08lx\n", sp);
}
void segv_handler()
{
int *c = NULL;
print_sp();
counter++;
printf("%d\n", counter);
if (counter == 23)
abort();
*c = 1; // SEGV
}
int main()
{
int *c = NULL;
char *s = malloc(SIGSTKSZ);
stack_t stack;
struct sigaction action;
memset(s, 0, SIGSTKSZ);
stack.ss_sp = s;
stack.ss_flags = 0;
stack.ss_size = SIGSTKSZ;
if (sigaltstack(&stack, NULL)) {
printf("Failed to use sigaltstack!\n");
return -1;
}
memset(&action, 0, sizeof(action));
action.sa_handler = segv_handler;
action.sa_flags = SA_ONSTACK | SA_NODEFER;
sigemptyset(&action.sa_mask);
sigaction(SIGSEGV, &action, NULL);
*c = 0; //SEGV
if (!s)
free(s);
return 0;
}
$ gcc test_sigaltstack.c -o test_sigaltstack
$ ./test_sigaltstack
sp = 0x120015c80
1
sp = 0x120015900
2
sp = 0x120015580
3
sp = 0x120015200
4
sp = 0x120014e80
5
sp = 0x120014b00
6
sp = 0x120014780
7
sp = 0x120014400
8
sp = 0x120014080
9
sp = 0x120013d00
10
sp = 0x120015c80
11 # wraparound occurs! the 11nd output is same as 1st.
sp = 0x120015900
12
sp = 0x120015580
13
sp = 0x120015200
14
sp = 0x120014e80
15
sp = 0x120014b00
16
sp = 0x120014780
17
sp = 0x120014400
18
sp = 0x120014080
19
sp = 0x120013d00
20
sp = 0x120015c80
21 # wraparound occurs! the 21nd output is same as 1st.
sp = 0x120015900
22
sp = 0x120015580
23
Aborted
With this patch:
$ ./test_sigaltstack
sp = 0x120015c80
1
sp = 0x120015900
2
sp = 0x120015580
3
sp = 0x120015200
4
sp = 0x120014e80
5
sp = 0x120014b00
6
sp = 0x120014780
7
sp = 0x120014400
8
sp = 0x120014080
9
Segmentation fault
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.
This patch is similar with commit 83bd01024b1f ("x86: protect against
sigaltstack wraparound").
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
arch/mips/kernel/signal.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index c9b2a75..c1632e8 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -563,6 +563,13 @@ void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
sp = regs->regs[29];
/*
+ * 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 (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
+ return (void __user __force *)(-1UL);
+
+ /*
* FPU emulator may have it's own trampoline active just
* above the user stack, 16-bytes before the next lowest
* 16 byte boundary. Try to avoid trashing it.
--
2.1.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/3] MIPS: signal: Protect against sigaltstack wraparound
2021-12-20 4:27 ` [PATCH v2 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
@ 2022-01-02 13:31 ` Thomas Bogendoerfer
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Bogendoerfer @ 2022-01-02 13:31 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: Xuefeng Li, linux-mips, linux-kernel
On Mon, Dec 20, 2021 at 12:27:38PM +0800, Tiezhu Yang wrote:
> If a process uses alternative signal stack by using sigaltstack(),
> then that stack overflows and stack wraparound occurs.
>
> Simple Explanation:
> The accurate sp order is A,B,C,D,...
> But now the sp points to A,B,C and A,B,C again.
>
> This problem can reproduce by the following code:
>
> $ cat test_sigaltstack.c
> #include <stdio.h>
> #include <signal.h>
> #include <stdlib.h>
> #include <string.h>
>
> volatile int counter = 0;
>
> void print_sp()
> {
> unsigned long sp;
>
> __asm__ __volatile__("move %0, $sp" : "=r" (sp));
> printf("sp = 0x%08lx\n", sp);
> }
>
> void segv_handler()
> {
> int *c = NULL;
>
> print_sp();
> counter++;
> printf("%d\n", counter);
>
> if (counter == 23)
> abort();
>
> *c = 1; // SEGV
> }
>
> int main()
> {
> int *c = NULL;
> char *s = malloc(SIGSTKSZ);
> stack_t stack;
> struct sigaction action;
>
> memset(s, 0, SIGSTKSZ);
> stack.ss_sp = s;
> stack.ss_flags = 0;
> stack.ss_size = SIGSTKSZ;
> if (sigaltstack(&stack, NULL)) {
> printf("Failed to use sigaltstack!\n");
> return -1;
> }
>
> memset(&action, 0, sizeof(action));
> action.sa_handler = segv_handler;
> action.sa_flags = SA_ONSTACK | SA_NODEFER;
> sigemptyset(&action.sa_mask);
> sigaction(SIGSEGV, &action, NULL);
>
> *c = 0; //SEGV
>
> if (!s)
> free(s);
>
> return 0;
> }
> $ gcc test_sigaltstack.c -o test_sigaltstack
> $ ./test_sigaltstack
> sp = 0x120015c80
> 1
> sp = 0x120015900
> 2
> sp = 0x120015580
> 3
> sp = 0x120015200
> 4
> sp = 0x120014e80
> 5
> sp = 0x120014b00
> 6
> sp = 0x120014780
> 7
> sp = 0x120014400
> 8
> sp = 0x120014080
> 9
> sp = 0x120013d00
> 10
> sp = 0x120015c80
> 11 # wraparound occurs! the 11nd output is same as 1st.
> sp = 0x120015900
> 12
> sp = 0x120015580
> 13
> sp = 0x120015200
> 14
> sp = 0x120014e80
> 15
> sp = 0x120014b00
> 16
> sp = 0x120014780
> 17
> sp = 0x120014400
> 18
> sp = 0x120014080
> 19
> sp = 0x120013d00
> 20
> sp = 0x120015c80
> 21 # wraparound occurs! the 21nd output is same as 1st.
> sp = 0x120015900
> 22
> sp = 0x120015580
> 23
> Aborted
>
> With this patch:
>
> $ ./test_sigaltstack
> sp = 0x120015c80
> 1
> sp = 0x120015900
> 2
> sp = 0x120015580
> 3
> sp = 0x120015200
> 4
> sp = 0x120014e80
> 5
> sp = 0x120014b00
> 6
> sp = 0x120014780
> 7
> sp = 0x120014400
> 8
> sp = 0x120014080
> 9
> Segmentation fault
>
> 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.
>
> This patch is similar with commit 83bd01024b1f ("x86: protect against
> sigaltstack wraparound").
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
> arch/mips/kernel/signal.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index c9b2a75..c1632e8 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -563,6 +563,13 @@ void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
> sp = regs->regs[29];
>
> /*
> + * 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 (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
> + return (void __user __force *)(-1UL);
> +
> + /*
> * FPU emulator may have it's own trampoline active just
> * above the user stack, 16-bytes before the next lowest
> * 16 byte boundary. Try to avoid trashing it.
> --
> 2.1.0
applied to mips-next.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] MIPS: signal: Return immediately if call fails
2021-12-20 4:27 [PATCH v2 0/3] MIPS: signal: Modify some code Tiezhu Yang
2021-12-20 4:27 ` [PATCH v2 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
@ 2021-12-20 4:27 ` Tiezhu Yang
2022-01-02 13:31 ` Thomas Bogendoerfer
2021-12-20 4:27 ` [PATCH v2 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code Tiezhu Yang
2 siblings, 1 reply; 8+ messages in thread
From: Tiezhu Yang @ 2021-12-20 4:27 UTC (permalink / raw)
To: Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel
When debug sigaltstack(), copy_siginfo_to_user() fails first in
setup_rt_frame() if the alternate signal stack is too small, so
it should return immediately if call fails, no need to call the
following functions.
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
arch/mips/kernel/signal.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index c1632e8..5bce782 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -754,23 +754,25 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
struct pt_regs *regs, sigset_t *set)
{
struct rt_sigframe __user *frame;
- int err = 0;
frame = get_sigframe(ksig, regs, sizeof(*frame));
if (!access_ok(frame, sizeof (*frame)))
return -EFAULT;
/* Create siginfo. */
- err |= copy_siginfo_to_user(&frame->rs_info, &ksig->info);
+ if (copy_siginfo_to_user(&frame->rs_info, &ksig->info))
+ return -EFAULT;
/* Create the ucontext. */
- err |= __put_user(0, &frame->rs_uc.uc_flags);
- err |= __put_user(NULL, &frame->rs_uc.uc_link);
- err |= __save_altstack(&frame->rs_uc.uc_stack, regs->regs[29]);
- err |= setup_sigcontext(regs, &frame->rs_uc.uc_mcontext);
- err |= __copy_to_user(&frame->rs_uc.uc_sigmask, set, sizeof(*set));
-
- if (err)
+ if (__put_user(0, &frame->rs_uc.uc_flags))
+ return -EFAULT;
+ if (__put_user(NULL, &frame->rs_uc.uc_link))
+ return -EFAULT;
+ if (__save_altstack(&frame->rs_uc.uc_stack, regs->regs[29]))
+ return -EFAULT;
+ if (setup_sigcontext(regs, &frame->rs_uc.uc_mcontext))
+ return -EFAULT;
+ if (__copy_to_user(&frame->rs_uc.uc_sigmask, set, sizeof(*set)))
return -EFAULT;
/*
--
2.1.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] MIPS: signal: Return immediately if call fails
2021-12-20 4:27 ` [PATCH v2 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
@ 2022-01-02 13:31 ` Thomas Bogendoerfer
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Bogendoerfer @ 2022-01-02 13:31 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: Xuefeng Li, linux-mips, linux-kernel
On Mon, Dec 20, 2021 at 12:27:39PM +0800, Tiezhu Yang wrote:
> When debug sigaltstack(), copy_siginfo_to_user() fails first in
> setup_rt_frame() if the alternate signal stack is too small, so
> it should return immediately if call fails, no need to call the
> following functions.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
> arch/mips/kernel/signal.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index c1632e8..5bce782 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -754,23 +754,25 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
> struct pt_regs *regs, sigset_t *set)
> {
> struct rt_sigframe __user *frame;
> - int err = 0;
>
> frame = get_sigframe(ksig, regs, sizeof(*frame));
> if (!access_ok(frame, sizeof (*frame)))
> return -EFAULT;
>
> /* Create siginfo. */
> - err |= copy_siginfo_to_user(&frame->rs_info, &ksig->info);
> + if (copy_siginfo_to_user(&frame->rs_info, &ksig->info))
> + return -EFAULT;
>
> /* Create the ucontext. */
> - err |= __put_user(0, &frame->rs_uc.uc_flags);
> - err |= __put_user(NULL, &frame->rs_uc.uc_link);
> - err |= __save_altstack(&frame->rs_uc.uc_stack, regs->regs[29]);
> - err |= setup_sigcontext(regs, &frame->rs_uc.uc_mcontext);
> - err |= __copy_to_user(&frame->rs_uc.uc_sigmask, set, sizeof(*set));
> -
> - if (err)
> + if (__put_user(0, &frame->rs_uc.uc_flags))
> + return -EFAULT;
> + if (__put_user(NULL, &frame->rs_uc.uc_link))
> + return -EFAULT;
> + if (__save_altstack(&frame->rs_uc.uc_stack, regs->regs[29]))
> + return -EFAULT;
> + if (setup_sigcontext(regs, &frame->rs_uc.uc_mcontext))
> + return -EFAULT;
> + if (__copy_to_user(&frame->rs_uc.uc_sigmask, set, sizeof(*set)))
> return -EFAULT;
>
> /*
> --
> 2.1.0
applied to mips-next.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code
2021-12-20 4:27 [PATCH v2 0/3] MIPS: signal: Modify some code Tiezhu Yang
2021-12-20 4:27 ` [PATCH v2 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
2021-12-20 4:27 ` [PATCH v2 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
@ 2021-12-20 4:27 ` Tiezhu Yang
2022-01-02 13:32 ` Thomas Bogendoerfer
2 siblings, 1 reply; 8+ messages in thread
From: Tiezhu Yang @ 2021-12-20 4:27 UTC (permalink / raw)
To: Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel
DEBUG_SIG is not defined on MIPS, so DEBUGP() is an empty function.
Additionally, it is unacceptable to printk messages in the normal
path of signal handling, the system can not work well if DEBUG_SIG
is defined, so just remove the related code.
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
arch/mips/kernel/signal-common.h | 8 --------
arch/mips/kernel/signal.c | 7 -------
arch/mips/kernel/signal_n32.c | 4 ----
arch/mips/kernel/signal_o32.c | 8 --------
4 files changed, 27 deletions(-)
diff --git a/arch/mips/kernel/signal-common.h b/arch/mips/kernel/signal-common.h
index f50d484..f70135f 100644
--- a/arch/mips/kernel/signal-common.h
+++ b/arch/mips/kernel/signal-common.h
@@ -11,14 +11,6 @@
#ifndef __SIGNAL_COMMON_H
#define __SIGNAL_COMMON_H
-/* #define DEBUG_SIG */
-
-#ifdef DEBUG_SIG
-# define DEBUGP(fmt, args...) printk("%s: " fmt, __func__, ##args)
-#else
-# define DEBUGP(fmt, args...)
-#endif
-
/*
* Determine which stack to use..
*/
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 5bce782..ca95211 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -743,9 +743,6 @@ static int setup_frame(void *sig_return, struct ksignal *ksig,
regs->regs[31] = (unsigned long) sig_return;
regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
- DEBUGP("SIG deliver (%s:%d): sp=0x%p pc=0x%lx ra=0x%lx\n",
- current->comm, current->pid,
- frame, regs->cp0_epc, regs->regs[31]);
return 0;
}
#endif
@@ -792,10 +789,6 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
regs->regs[31] = (unsigned long) sig_return;
regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
- DEBUGP("SIG deliver (%s:%d): sp=0x%p pc=0x%lx ra=0x%lx\n",
- current->comm, current->pid,
- frame, regs->cp0_epc, regs->regs[31]);
-
return 0;
}
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index 7bd00fa..d0e3f74 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -130,10 +130,6 @@ static int setup_rt_frame_n32(void *sig_return, struct ksignal *ksig,
regs->regs[31] = (unsigned long) sig_return;
regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
- DEBUGP("SIG deliver (%s:%d): sp=0x%p pc=0x%lx ra=0x%lx\n",
- current->comm, current->pid,
- frame, regs->cp0_epc, regs->regs[31]);
-
return 0;
}
diff --git a/arch/mips/kernel/signal_o32.c b/arch/mips/kernel/signal_o32.c
index 299a7a2..3691f74 100644
--- a/arch/mips/kernel/signal_o32.c
+++ b/arch/mips/kernel/signal_o32.c
@@ -144,10 +144,6 @@ static int setup_frame_32(void *sig_return, struct ksignal *ksig,
regs->regs[31] = (unsigned long) sig_return;
regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
- DEBUGP("SIG deliver (%s:%d): sp=0x%p pc=0x%lx ra=0x%lx\n",
- current->comm, current->pid,
- frame, regs->cp0_epc, regs->regs[31]);
-
return 0;
}
@@ -230,10 +226,6 @@ static int setup_rt_frame_32(void *sig_return, struct ksignal *ksig,
regs->regs[31] = (unsigned long) sig_return;
regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
- DEBUGP("SIG deliver (%s:%d): sp=0x%p pc=0x%lx ra=0x%lx\n",
- current->comm, current->pid,
- frame, regs->cp0_epc, regs->regs[31]);
-
return 0;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code
2021-12-20 4:27 ` [PATCH v2 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code Tiezhu Yang
@ 2022-01-02 13:32 ` Thomas Bogendoerfer
2022-01-18 16:01 ` Maciej W. Rozycki
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Bogendoerfer @ 2022-01-02 13:32 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: Xuefeng Li, linux-mips, linux-kernel
On Mon, Dec 20, 2021 at 12:27:40PM +0800, Tiezhu Yang wrote:
> DEBUG_SIG is not defined on MIPS, so DEBUGP() is an empty function.
> Additionally, it is unacceptable to printk messages in the normal
> path of signal handling, the system can not work well if DEBUG_SIG
> is defined, so just remove the related code.
I like to keep this debug aid for now.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code
2022-01-02 13:32 ` Thomas Bogendoerfer
@ 2022-01-18 16:01 ` Maciej W. Rozycki
0 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2022-01-18 16:01 UTC (permalink / raw)
To: Thomas Bogendoerfer; +Cc: Tiezhu Yang, Xuefeng Li, linux-mips, linux-kernel
On Sun, 2 Jan 2022, Thomas Bogendoerfer wrote:
> > DEBUG_SIG is not defined on MIPS, so DEBUGP() is an empty function.
> > Additionally, it is unacceptable to printk messages in the normal
> > path of signal handling, the system can not work well if DEBUG_SIG
> > is defined, so just remove the related code.
>
> I like to keep this debug aid for now.
And it is perhaps worth noting that we have similar developer aids in
many places across our code base.
Maciej
^ permalink raw reply [flat|nested] 8+ messages in thread