From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42c.google.com (mail-pf1-x42c.google.com [IPv6:2607:f8b0:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id 893DD3858C2D for ; Wed, 3 Aug 2022 17:27:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 893DD3858C2D Received: by mail-pf1-x42c.google.com with SMTP id 17so17123941pfy.0 for ; Wed, 03 Aug 2022 10:27:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gXzexOFGF8ImfMGnHXbm9fkruIgJ1IXCht5ZJaHpAMs=; b=EVkVlfja/HGrJ/1NKYcTxXcxS0Lud3IH73CSyaGtPqBcQcGvZK+ZA0bKwbf72NOVjb u6P7QXS262x0uNyZSaaLrwjhUp8i+XCpXCQGQ1EUGXX82pmYmzz2+z/Km3xWA7i7GcXI xpgWmb+4wsWzSY/D9MWivHHYR0btp2t+I8BTp0Fxpr0meOCRGARSQ7ePgmImTRDYiWbx oC4O0DVRkNISFURaAt8t1blPgxh9PhHDOfKgWpnyrIjF0JTiexF5V/Rmc+//sTueat+t JBgDLWOV5ZYYw9Fzwl/AMngFPohgsed03YttXdctuaxtl33/skMp2E1a5FcD7tRb3Hje C0eQ== X-Gm-Message-State: ACgBeo1zYFjV2DWWoPNE1LzhK9hyBrh6QA5sJCNEOmBNb2yb9ndy+feW B+HNPTUhguA/W6VQ7ztWvyGu5msQvh0yM1K1JKE= X-Google-Smtp-Source: AA6agR4ZSHvCBRdNul+/zzx4kF9LeiCd9uc6ewscYDj/1kwd0B8WJrA85/WjI/KdGAJsT5PvJh7XS96bc8ecmwLJavA= X-Received: by 2002:a63:82c2:0:b0:41b:c0f3:39b3 with SMTP id w185-20020a6382c2000000b0041bc0f339b3mr16979496pgd.86.1659547665342; Wed, 03 Aug 2022 10:27:45 -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, 3 Aug 2022 10:27:09 -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.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP 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, 03 Aug 2022 17:27:48 -0000 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 (); > jeff > > > > -- > > H.J. > -- H.J.