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

[Andy Lutomirski <luto@kernel.org>]

On Wed, Feb 3, 2021 at 9:27 AM 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.
>
> While previous patches in this series allow 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.
>
> Suggested-by: Jann Horn <jannh@google.com>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Reviewed-by: Jann Horn <jannh@google.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Jann Horn <jannh@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Changes from v3:
> * Updated the changelog (Borislav Petkov)
>
> Changes from v2:
> * Simplified the implementation (Jann Horn)
> ---
>  arch/x86/kernel/signal.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 0d24f64d0145..8e2df070dbfd 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -242,7 +242,7 @@ 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 onsigstack = on_sig_stack(sp);
>         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;

This is buggy.  The old code had a dubious special case for
SS_AUTODISARM, and this interacts poorly with it.  I think you could
fix it by separating the case in which you are already on the altstack
from the case in which you're switching to the altstack, or you could
fix it by changing the check at the end of the function to literally
check whether the sp value is in bounds instead of calling
on_sig_stack.

Arguably the generic helpers could be adjusted to make this less annoying.