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

On Mon, Mar 15, 2021 at 11:57 PM Chang S. Bae [off-list ref] 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".

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).

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.