[PATCH 0/3] MIPS: signal: Modify some code

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/3] MIPS: signal: Modify some code
@ 2021-12-18  3:23 Tiezhu Yang
  2021-12-18  3:23 ` [PATCH 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Tiezhu Yang @ 2021-12-18  3:23 UTC (permalink / raw)
  To: Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel

Tiezhu Yang (3):
  MIPS: signal: Protect against sigaltstack wraparound
  MIPS: signal: Return immediately if call fails
  MIPS: signal: Remove unnecessary DEBUG_SIG related code

 arch/mips/kernel/signal-common.h |  8 --------
 arch/mips/kernel/signal.c        | 39 ++++++++++++++++++++++++++-------------
 arch/mips/kernel/signal_n32.c    |  4 ----
 arch/mips/kernel/signal_o32.c    |  8 --------
 4 files changed, 26 insertions(+), 33 deletions(-)

-- 
2.1.0


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

* [PATCH 1/3] MIPS: signal: Protect against sigaltstack wraparound
  2021-12-18  3:23 [PATCH 0/3] MIPS: signal: Modify some code Tiezhu Yang
@ 2021-12-18  3:23 ` Tiezhu Yang
  2021-12-18  3:23 ` [PATCH 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
  2021-12-18  3:23 ` [PATCH 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code Tiezhu Yang
  2 siblings, 0 replies; 6+ messages in thread
From: Tiezhu Yang @ 2021-12-18  3:23 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] 6+ messages in thread

* [PATCH 2/3] MIPS: signal: Return immediately if call fails
  2021-12-18  3:23 [PATCH 0/3] MIPS: signal: Modify some code Tiezhu Yang
  2021-12-18  3:23 ` [PATCH 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
@ 2021-12-18  3:23 ` Tiezhu Yang
  2021-12-18 14:08   ` Jiaxun Yang
  2021-12-18  3:23 ` [PATCH 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code Tiezhu Yang
  2 siblings, 1 reply; 6+ messages in thread
From: Tiezhu Yang @ 2021-12-18  3:23 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 | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index c1632e8..4cd3969 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -761,15 +761,28 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
 		return -EFAULT;
 
 	/* Create siginfo.  */
-	err |= copy_siginfo_to_user(&frame->rs_info, &ksig->info);
+	err = copy_siginfo_to_user(&frame->rs_info, &ksig->info);
+	if (err)
+		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));
+	err = __put_user(0, &frame->rs_uc.uc_flags);
+	if (err)
+		return -EFAULT;
+
+	err = __put_user(NULL, &frame->rs_uc.uc_link);
+	if (err)
+		return -EFAULT;
+
+	err = __save_altstack(&frame->rs_uc.uc_stack, regs->regs[29]);
+	if (err)
+		return -EFAULT;
+
+	err = setup_sigcontext(regs, &frame->rs_uc.uc_mcontext);
+	if (err)
+		return -EFAULT;
 
+	err = __copy_to_user(&frame->rs_uc.uc_sigmask, set, sizeof(*set));
 	if (err)
 		return -EFAULT;
 
-- 
2.1.0


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

* Re: [PATCH 2/3] MIPS: signal: Return immediately if call fails
  2021-12-18  3:23 ` [PATCH 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
@ 2021-12-18 14:08   ` Jiaxun Yang
  2021-12-20  2:25     ` Tiezhu Yang
  0 siblings, 1 reply; 6+ messages in thread
From: Jiaxun Yang @ 2021-12-18 14:08 UTC (permalink / raw)
  To: Tiezhu Yang, Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel



在 2021/12/18 3:23, Tiezhu Yang 写道:
> 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.

Hi Tiezhu,

Thanks for your patch.
If we are doing so I see no reason for keeping the err variable.
Just

if (copy_siginfo_to_user(&frame->rs_info, &ksig->info))
     return -EFAULT;

seems much more clear.

Thanks.

- Jiaxun

>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/mips/kernel/signal.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index c1632e8..4cd3969 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -761,15 +761,28 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
>   		return -EFAULT;
>   
>   	/* Create siginfo.  */
> -	err |= copy_siginfo_to_user(&frame->rs_info, &ksig->info);
> +	err = copy_siginfo_to_user(&frame->rs_info, &ksig->info);
> +	if (err)
> +		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));
> +	err = __put_user(0, &frame->rs_uc.uc_flags);
> +	if (err)
> +		return -EFAULT;
> +
> +	err = __put_user(NULL, &frame->rs_uc.uc_link);
> +	if (err)
> +		return -EFAULT;
> +
> +	err = __save_altstack(&frame->rs_uc.uc_stack, regs->regs[29]);
> +	if (err)
> +		return -EFAULT;
> +
> +	err = setup_sigcontext(regs, &frame->rs_uc.uc_mcontext);
> +	if (err)
> +		return -EFAULT;
>   
> +	err = __copy_to_user(&frame->rs_uc.uc_sigmask, set, sizeof(*set));
>   	if (err)
>   		return -EFAULT;
>   


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

* Re: [PATCH 2/3] MIPS: signal: Return immediately if call fails
  2021-12-18 14:08   ` Jiaxun Yang
@ 2021-12-20  2:25     ` Tiezhu Yang
  0 siblings, 0 replies; 6+ messages in thread
From: Tiezhu Yang @ 2021-12-20  2:25 UTC (permalink / raw)
  To: Jiaxun Yang, Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel

On 12/18/2021 10:08 PM, Jiaxun Yang wrote:
>
>
> 在 2021/12/18 3:23, Tiezhu Yang 写道:
>> 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.
>
> Hi Tiezhu,
>
> Thanks for your patch.
> If we are doing so I see no reason for keeping the err variable.
> Just
>
> if (copy_siginfo_to_user(&frame->rs_info, &ksig->info))
>     return -EFAULT;
>
> seems much more clear.

OK, thank you, I will send v2 later.

Thanks,
Tiezhu

>
> Thanks.
>
> - Jiaxun
>


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

* [PATCH 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code
  2021-12-18  3:23 [PATCH 0/3] MIPS: signal: Modify some code Tiezhu Yang
  2021-12-18  3:23 ` [PATCH 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
  2021-12-18  3:23 ` [PATCH 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
@ 2021-12-18  3:23 ` Tiezhu Yang
  2 siblings, 0 replies; 6+ messages in thread
From: Tiezhu Yang @ 2021-12-18  3:23 UTC (permalink / raw)
  To: Thomas Bogendoerfer; +Cc: Xuefeng Li, linux-mips, linux-kernel

Since DEBUG_SIG is not defined on MIPS, so DEBUGP() is an empty function.
Additionally, it is unacceptable to printk message 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 4cd3969..feb0cba 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
@@ -803,10 +800,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] 6+ messages in thread

end of thread, other threads:[~2021-12-20  2:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-18  3:23 [PATCH 0/3] MIPS: signal: Modify some code Tiezhu Yang
2021-12-18  3:23 ` [PATCH 1/3] MIPS: signal: Protect against sigaltstack wraparound Tiezhu Yang
2021-12-18  3:23 ` [PATCH 2/3] MIPS: signal: Return immediately if call fails Tiezhu Yang
2021-12-18 14:08   ` Jiaxun Yang
2021-12-20  2:25     ` Tiezhu Yang
2021-12-18  3:23 ` [PATCH 3/3] MIPS: signal: Remove unnecessary DEBUG_SIG related code Tiezhu Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).