Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow

[Andy Lutomirski <luto@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5557 bytes --]

On Mon, Mar 15, 2021 at 11:57 PM Chang S. Bae <chang.seok.bae@intel.com> 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.
>
> Instead of calling on_sig_stack(), directly check the new stack pointer
> whether in the bounds.
>
> 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.

This patch results in excessively complicated control and data flow.

> -       int onsigstack = on_sig_stack(sp);
> +       bool onsigstack = on_sig_stack(sp);

Here onsigstack means "we were already using the altstack".

>         int ret;
>
>         /* redzone */
> @@ -251,8 +251,11 @@ 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;
> +                       /* On the alternate signal stack */
> +                       onsigstack = true;
> +               }

But now onsigstack is also true if we are using the legacy path to
*enter* the altstack.  So now it's (was on altstack) || (entering
altstack via legacy path).

>         } else if (IS_ENABLED(CONFIG_X86_32) &&
>                    !onsigstack &&
>                    regs->ss != __USER_DS &&
> @@ -272,7 +275,8 @@ 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 (onsigstack && unlikely(sp <= current->sas_ss_sp ||
> +                                  sp - current->sas_ss_sp > current->sas_ss_size))

And now we fail if ((was on altstack) || (entering altstack via legacy
path)) && (new sp is out of bounds).


The condition we actually want is that, if we are entering the
altstack and we don't fit, we should fail.  This is tricky because of
the autodisarm stuff and the possibility of nonlinear stack segments,
so it's not even clear to me exactly what we should be doing.  I
propose:




>                 return (void __user *)-1L;

Can we please log something (if (show_unhandled_signals ||
printk_ratelimit()) that says that we overflowed the altstack?

How about:

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ea794a083c44..53781324a2d3 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -237,7 +237,8 @@ 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 already_onsigstack = on_sig_stack(sp);
+    bool entering_altstack = false;
     int ret;

     /* redzone */
@@ -246,15 +247,25 @@ 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 already_onsigstack via sas_ss_flags().
+         * Sensible programs use SS_AUTODISARM, which disables
+         * that check, and programs that don't use
+         * SS_AUTODISARM get compatible but potentially
+         * bizarre behavior.
+         */
+        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 &&
+           !already_onsigstack &&
            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),
@@ -267,8 +278,16 @@ 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(entering_altstack &&
+             (sp <= current->sas_ss_sp ||
+              sp - current->sas_ss_sp > current->sas_ss_size))) {
+        if (show_unhandled_signals && printk_ratelimit()) {
+            pr_info("%s[%d] overflowed sigaltstack",
+                tsk->comm, task_pid_nr(tsk));
+        }
+
         return (void __user *)-1L;
+    }

     /* save i387 and extended state */
     ret = copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size);

Apologies for whitespace damage.  I attached it, too.

[-- Attachment #2: stack.patch --]
[-- Type: text/x-patch, Size: 2212 bytes --]

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ea794a083c44..53781324a2d3 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -237,7 +237,8 @@ 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 already_onsigstack = on_sig_stack(sp);
+	bool entering_altstack = false;
 	int ret;
 
 	/* redzone */
@@ -246,15 +247,25 @@ 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 already_onsigstack via sas_ss_flags().
+		 * Sensible programs use SS_AUTODISARM, which disables
+		 * that check, and programs that don't use
+		 * SS_AUTODISARM get compatible but potentially
+		 * bizarre behavior.
+		 */
+		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 &&
+		   !already_onsigstack &&
 		   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),
@@ -267,8 +278,16 @@ 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(entering_altstack &&
+		     (sp <= current->sas_ss_sp ||
+		      sp - current->sas_ss_sp > current->sas_ss_size))) {
+		if (show_unhandled_signals && printk_ratelimit()) {
+			pr_info("%s[%d] overflowed sigaltstack",
+				tsk->comm, task_pid_nr(tsk));
+		}
+
 		return (void __user *)-1L;
+	}
 
 	/* save i387 and extended state */
 	ret = copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size);