From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CC4EDC43381 for ; Fri, 19 Feb 2021 01:26:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8E59564ECA for ; Fri, 19 Feb 2021 01:26:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229700AbhBSB0y (ORCPT ); Thu, 18 Feb 2021 20:26:54 -0500 Received: from mail.kernel.org ([198.145.29.99]:43722 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229639AbhBSB0w (ORCPT ); Thu, 18 Feb 2021 20:26:52 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id F0FDD64EDE for ; Fri, 19 Feb 2021 01:26:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1613697971; bh=fJwtJB/HDR3ilN4vGWzecz9MdjUktr8sE8/8Lj+agnk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=NuoPwIT6jPCvq7CVpE/RtirLulnNpdzGlK68XaOpM3R2YKEuoOh4eUnIJ144K9FeD 6mIwP/zrONWtPFo6OHrcsNp1aXfjgcNLWNwFLsX3H5Scj914pyJNKvFBWOWApvATtw 7h6NlWL1eYUnrkN9HRAyHFOdSPrb+CXmf0yr8Sp5OFGWvade7pZLgsqt0ht7KqatCl CiqO6oW2LnJtN0PrpEbJXPdv8Zp4RFmd17DwZFZfaXQFJu3ESBjCDyEhmhcY+0jUQ/ 4uGPniGZBe40KP38/d4NEbZM3D1QtWNuvnMzHnZu3f71z+GXGjUUcC3uYpTQQdwWQ0 xFTMhRK9xtSIQ== Received: by mail-ed1-f47.google.com with SMTP id i14so6430735eds.8 for ; Thu, 18 Feb 2021 17:26:10 -0800 (PST) X-Gm-Message-State: AOAM531y22s/RVvbgklrANkXS+P1G5yonSxI1wyqQTxh+4/SqtbgUGmv 3U1ufv5BCbUPbWJ8EN+em4coKOuCXXABupjpWO8crg== X-Google-Smtp-Source: ABdhPJz9RKmMCDP9wSoKObG17qi1NUro62wHOW6LLChZ1Op/hEl2UrY5iiGTy+gNcjBpcwId0RTiDEISDMvJE8ODtTI= X-Received: by 2002:a05:6402:1bc7:: with SMTP id ch7mr6925128edb.84.1613697969219; Thu, 18 Feb 2021 17:26:09 -0800 (PST) MIME-Version: 1.0 References: <20210203172242.29644-1-chang.seok.bae@intel.com> <20210203172242.29644-5-chang.seok.bae@intel.com> In-Reply-To: <20210203172242.29644-5-chang.seok.bae@intel.com> From: Andy Lutomirski Date: Thu, 18 Feb 2021 17:25:58 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 4/5] x86/signal: Detect and prevent an alternate signal stack overflow To: "Chang S. Bae" Cc: Borislav Petkov , Thomas Gleixner , Ingo Molnar , Andrew Lutomirski , X86 ML , Len Brown , Dave Hansen , "H. J. Lu" , Dave Martin , Jann Horn , Michael Ellerman , "Carlos O'Donell" , Tony Luck , "Ravi V. Shankar" , libc-alpha , linux-arch , Linux API , LKML , Borislav Petkov Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 3, 2021 at 9:27 AM Chang S. Bae 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 > Signed-off-by: Chang S. Bae > Reviewed-by: Len Brown > Reviewed-by: Jann Horn > Cc: Borislav Petkov > Cc: Jann Horn > 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.