From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x733.google.com (mail-qk1-x733.google.com [IPv6:2607:f8b0:4864:20::733]) by sourceware.org (Postfix) with ESMTPS id 603573858D1E for ; Wed, 17 Aug 2022 22:20:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 603573858D1E Received: by mail-qk1-x733.google.com with SMTP id m5so11464458qkk.1 for ; Wed, 17 Aug 2022 15:20:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=664K/+anCktCBwwzja1hL7BKc+43fjyjsWwHmFb7lDY=; b=OXjiBLtjCMnL075pQ/SeMd6y2ggwsOrf6XkBeR1gMHbc3KS/CXmjHpPwElxskf5995 Tzt+w7eqQyEz44kuQ6+WIdtrV7Kl1xy2C0DofoOJnQF72h10cJeEf2J0TMvpgW5crWL2 MWKzrA56ckpEbNx1PrOmyTTUKjfBZG5VkbN7frEQBdwPFqfbLaGcYUeforlOrpwExj4F x2Pryt4rnqS6t9cT+z1WYRVQchk3rMHi6MY/4M0kbvpuSa+wb0APtsGGA1BZRRE/JvZ0 i7jxqRaGfvvip+vS9LXc/50kdy60gM0mDvHXl2FKUPlFrjwjZs8Git/YdgPcL+IWSx4t v2+w== X-Gm-Message-State: ACgBeo1EjUNyLF90LbfpjVSOtpC/hg9NP6aIiRU07FRHKcNoKqCzjMMc mIyhprsA6+205I1EuKExQiYL1iD4HrT4v7g08Yk= X-Google-Smtp-Source: AA6agR5iqZfTnVGoIc7X6KzEJe9rPMTn0FapGwuHpjBGhKx+dueZIPJkIZ3TOBkqQgWRuHEylFBJX+j5TdpVMm56A5U= X-Received: by 2002:a05:620a:2a0a:b0:6b5:e22d:7c4c with SMTP id o10-20020a05620a2a0a00b006b5e22d7c4cmr124554qkp.477.1660774829570; Wed, 17 Aug 2022 15:20:29 -0700 (PDT) MIME-Version: 1.0 References: <20220714215522.359952-1-hjl.tools@gmail.com> <8088866d-2dd2-35ff-587b-567cb60db84e@gmail.com> In-Reply-To: From: "H.J. Lu" Date: Wed, 17 Aug 2022 15:19:53 -0700 Message-ID: Subject: Re: [PATCH] stack-protector: Check stack canary for noreturn function To: Jeff Law Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3018.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Aug 2022 22:20:31 -0000 On Wed, Aug 3, 2022 at 10:27 AM H.J. Lu wrote: > > On Tue, Aug 2, 2022 at 4:34 PM Jeff Law wrote: > > > > > > > > On 8/2/2022 11:43 AM, H.J. Lu wrote: > > > On Sat, Jul 30, 2022 at 1:30 PM Jeff Law via Gcc-patches > > > wrote: > > >> > > >> > > >> On 7/14/2022 3:55 PM, H.J. Lu via Gcc-patches wrote: > > >>> Check stack canary for noreturn function to catch stack corruption > > >>> before calling noreturn function. For C++, check stack canary when > > >>> throwing exception or resuming stack unwind to avoid corrupted stack. > > >>> > > >>> gcc/ > > >>> > > >>> PR middle-end/58245 > > >>> * calls.cc (expand_call): Check stack canary for noreturn > > >>> function. > > >>> > > >>> gcc/testsuite/ > > >>> > > >>> PR middle-end/58245 > > >>> * c-c++-common/pr58245-1.c: New test. > > >>> * g++.dg/pr58245-1.C: Likewise. > > >>> * g++.dg/fstack-protector-strong.C: Adjusted. > > >> But is this really something we want? I'd actually lean towards > > >> eliminating the useless load -- I don't necessarily think we should be > > >> treating non-returning paths specially here. > > >> > > >> The whole point of the stack protector is to prevent the *return* path > > >> from going to an attacker controlled location. I'm not sure checking > > >> the protector at this point actually does anything particularly useful. > > > throw is marked no return. Since the unwind library may read > > > the stack contents to unwind stack, it the stack is corrupted, the > > > exception handling may go wrong. Should we handle this case? > > That's the question I think we need to answer. The EH paths are a known > > security issue on Windows and while ours are notably different I'm not > > sure if there's a real attack surface in those paths. My sense is that > > if we need to tackle this that doing so on the throw side might be > > better as it's closer conceptually to when//how we check the canary for > > a normal return. > > Like this? > > @@ -3154,7 +3155,10 @@ expand_call (tree exp, rtx target, int ignore) > if (pass && (flags & ECF_MALLOC)) > start_sequence (); > > - if (pass == 0 > + /* Check the canary value for sibcall or function which doesn't > + return and could throw. */ > + if ((pass == 0 > + || ((flags & ECF_NORETURN) != 0 && tree_could_throw_p (exp))) > && crtl->stack_protect_guard > && targetm.stack_protect_runtime_enabled_p ()) > stack_protect_epilogue (); Here is the patch: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599916.html > > jeff > > > > > > -- > > > H.J. > > > > > -- > H.J. -- H.J.