From patchwork Tue Dec 10 09:27:15 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 1206990 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-515584-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="kbm3J2g1"; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="MmF/2DVQ"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47XF8X15T9z9sR0 for ; Tue, 10 Dec 2019 20:27:38 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:mime-version :content-type:content-transfer-encoding; q=dns; s=default; b=o+s nzuB1QFkUa4dys8NnqrVkh9ICoUK4jN17FMFXgRc7LTZWO2GRknyeNavAFDCJh/n 52fzv475sK84gNa2Etv1kpy+cVS1Plb85Tp0WWJTXoLbi+S1SLrmJ/4B/SZWs2vZ 3HFSeUP2G5Go6r6CoLqxeQOIRj1kKbShr8KCI9Zc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:mime-version :content-type:content-transfer-encoding; s=default; bh=Uf8oUwOuK e4sczxhYyrTWFxWmRU=; b=kbm3J2g1VDDgVij96Nwt9t5WMjMDwCImY+BFZMthd TJ2EAK8B+CaPP+wCmowW62pAY5v0vA2ehgdZv+Kl4mgJKoTI5iunDcIyulA0egqe D3SYiL/qWM7uaHcVDW721m9PeJjMaAfkizwciNIFHbitzNudL6jZ3du+nIu772Cj Hs= Received: (qmail 9893 invoked by alias); 10 Dec 2019 09:27:30 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 9883 invoked by uid 89); 10 Dec 2019 09:27:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-10.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.1 spammy=instruments, interference, techno, HTo:D*fr X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Dec 2019 09:27:27 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575970046; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=++8n6nfMcZT+Bcoykha87mNRY84VAJHmGgCcpSjGsBA=; b=MmF/2DVQ3LG0LMLVs3Ps8iTy6CL87CGnqKNcUECrh2hHpkKpjn+rQHD8ZplFPIFUqJJimX schFwPiCAc5lPU86QgzcJyuNzrOPhHTjUTaoMTyqCNUEUddZFi1wIxE8g54B2OgNVWf82F K88Ii0WHlbbwaXM3qm69102i8+lzk04= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-416-PQRvZnaBPUuNb8kslRwMsA-1; Tue, 10 Dec 2019 04:27:22 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 51BBB1005502; Tue, 10 Dec 2019 09:27:21 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-117-59.ams2.redhat.com [10.36.117.59]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B7D3760BE1; Tue, 10 Dec 2019 09:27:20 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id xBA9RI1x001907; Tue, 10 Dec 2019 10:27:18 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id xBA9RFlU001906; Tue, 10 Dec 2019 10:27:15 +0100 Date: Tue, 10 Dec 2019 10:27:15 +0100 From: Jakub Jelinek To: Jeff Law , Richard Biener , Eric Botcazou Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] Improve -fstack-protector-strong (PR middle-end/92825) Message-ID: <20191210092715.GX10088@tucnak> Reply-To: Jakub Jelinek MIME-Version: 1.0 User-Agent: Mutt/1.11.3 (2019-02-01) X-Mimecast-Spam-Score: 0 Content-Disposition: inline X-IsSubscribed: yes Hi! As mentioned in the PR, -fstack-protector-strong in some cases instruments even functions which except for the stack canary setup and later testing of that don't touch the stack at all. This is because for -fstack-protector-strong we setup the canary whenever stack_protect_decl_p returns true, and that goes through all local decls and if it is a var that is not a global var and is either address taken or is array or has arrays embedded in its type, returns true. Now, the problem is that by going through all non-global variables, it goes even through variables that are or have arrays, but we allocate them in registers like in the testcase below, or in theory for nested functions it could be variables from the parent function too. Variables which live in registers, even when they have arrays in them, shouldn't really cause stack overflows of any kind, out of bounds accesses to them never cause out of bounds stack accesses, usually they ought to be already optimized away, arrays that are accessed through non-constant indexes should cause variables to live in memory. Another issue is that the record_or_union_type_has_array_p function duplicates the behavior of stack_protect_classify_type when it returns the SPCT_HAS_ARRAY bit set. Initially, I thought I'd just move the stack_protect_decl_p call later when the stack_vars array is computed and instead of FOR_EACH_LOCAL_DECL walk the stack_vars array. But for the discovery of stack_vars that are or have arrays that is a waste of time, all such variables are already picked for phase 1 or phase 2 and thus has_protected_decls bool will be set if there are any and we already do create the stack canary if has_protected_decls is set. So, the only thing that remains is catch variables that aren't or don't contain arrays, but are address taken. Those go into the last unnumbered phase, but we still want to create stack canary. Instead of adding yet another walk I've done that in a function that already walks them. In addition to this, I've tried to clarify this in the documentation. For -fstack-protector, we've been doing it this way already before, optimized away or arrays living in registers didn't count, because we only walked stack-vars. And, the documentation also said that only > 8 byte arrays are protected with -stack-protector, but it is actually >= 8 byte (or whatever the ssp-buffer-size param is). Bootstrapped/regtested on x86_64-linux and i686-linux, in addition regtested (just check-gcc, check-c++-all and check-target-libstdc++-v3) with --target_board=unix/-fstack-protector-strong on both. Ok for trunk? 2019-12-10 Jakub Jelinek PR middle-end/92825 * cfgexpand.c (add_stack_protection_conflicts): Change return type from void to bool, return true if at least one stack_vars[i].decl is addressable. (record_or_union_type_has_array_p, stack_protect_decl_p): Remove. (expand_used_vars): Don't call stack_protect_decl_p, instead for -fstack-protector-strong set gen_stack_protect_signal to true if add_stack_protection_conflicts returned true. Formatting fixes. * doc/invoke.texi (-fstack-protector-strong): Clarify that optimized out variables or variables not living on the stack don't count. (-fstack-protector): Likewise. Clarify it affects >= 8 byte arrays rather than > 8 byte. * gcc.target/i386/pr92825.c: New test. Jakub --- gcc/cfgexpand.c.jj 2019-12-06 09:51:56.917899249 +0100 +++ gcc/cfgexpand.c 2019-12-09 13:29:16.518031095 +0100 @@ -1888,17 +1888,23 @@ asan_decl_phase_3 (size_t i) } /* Ensure that variables in different stack protection phases conflict - so that they are not merged and share the same stack slot. */ + so that they are not merged and share the same stack slot. + Return true if there are any address taken variables. */ -static void +static bool add_stack_protection_conflicts (void) { size_t i, j, n = stack_vars_num; unsigned char *phase; + bool ret = false; phase = XNEWVEC (unsigned char, n); for (i = 0; i < n; ++i) - phase[i] = stack_protect_decl_phase (stack_vars[i].decl); + { + phase[i] = stack_protect_decl_phase (stack_vars[i].decl); + if (TREE_ADDRESSABLE (stack_vars[i].decl)) + ret = true; + } for (i = 0; i < n; ++i) { @@ -1909,6 +1915,7 @@ add_stack_protection_conflicts (void) } XDELETEVEC (phase); + return ret; } /* Create a decl for the guard at the top of the stack frame. */ @@ -1993,50 +2000,6 @@ estimated_stack_frame_size (struct cgrap return estimated_poly_value (size); } -/* Helper routine to check if a record or union contains an array field. */ - -static int -record_or_union_type_has_array_p (const_tree tree_type) -{ - tree fields = TYPE_FIELDS (tree_type); - tree f; - - for (f = fields; f; f = DECL_CHAIN (f)) - if (TREE_CODE (f) == FIELD_DECL) - { - tree field_type = TREE_TYPE (f); - if (RECORD_OR_UNION_TYPE_P (field_type) - && record_or_union_type_has_array_p (field_type)) - return 1; - if (TREE_CODE (field_type) == ARRAY_TYPE) - return 1; - } - return 0; -} - -/* Check if the current function has local referenced variables that - have their addresses taken, contain an array, or are arrays. */ - -static bool -stack_protect_decl_p () -{ - unsigned i; - tree var; - - FOR_EACH_LOCAL_DECL (cfun, i, var) - if (!is_global_var (var)) - { - tree var_type = TREE_TYPE (var); - if (VAR_P (var) - && (TREE_CODE (var_type) == ARRAY_TYPE - || TREE_ADDRESSABLE (var) - || (RECORD_OR_UNION_TYPE_P (var_type) - && record_or_union_type_has_array_p (var_type)))) - return true; - } - return false; -} - /* Check if the current function has calls that use a return slot. */ static bool @@ -2103,8 +2066,7 @@ expand_used_vars (void) } if (flag_stack_protect == SPCT_FLAG_STRONG) - gen_stack_protect_signal - = stack_protect_decl_p () || stack_protect_return_slot_p (); + gen_stack_protect_signal = stack_protect_return_slot_p (); /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -2180,6 +2142,8 @@ expand_used_vars (void) if (stack_vars_num > 0) { + bool has_addressable_vars = false; + add_scope_conflicts (); /* If stack protection is enabled, we don't share space between @@ -2189,7 +2153,10 @@ expand_used_vars (void) || (flag_stack_protect == SPCT_FLAG_EXPLICIT && lookup_attribute ("stack_protect", DECL_ATTRIBUTES (current_function_decl))))) - add_stack_protection_conflicts (); + has_addressable_vars = add_stack_protection_conflicts (); + + if (flag_stack_protect == SPCT_FLAG_STRONG && has_addressable_vars) + gen_stack_protect_signal = true; /* Now that we have collected all stack variables, and have computed a minimal interference graph, attempt to save some stack space. */ @@ -2206,14 +2173,16 @@ expand_used_vars (void) case SPCT_FLAG_STRONG: if (gen_stack_protect_signal - || cfun->calls_alloca || has_protected_decls + || cfun->calls_alloca + || has_protected_decls || lookup_attribute ("stack_protect", DECL_ATTRIBUTES (current_function_decl))) create_stack_guard (); break; case SPCT_FLAG_DEFAULT: - if (cfun->calls_alloca || has_protected_decls + if (cfun->calls_alloca + || has_protected_decls || lookup_attribute ("stack_protect", DECL_ATTRIBUTES (current_function_decl))) create_stack_guard (); @@ -2224,8 +2193,9 @@ expand_used_vars (void) DECL_ATTRIBUTES (current_function_decl))) create_stack_guard (); break; + default: - ; + break; } /* Assign rtl to each variable based on these partitions. */ --- gcc/doc/invoke.texi.jj 2019-12-06 00:40:39.468705299 +0100 +++ gcc/doc/invoke.texi 2019-12-09 13:44:16.172408953 +0100 @@ -13006,9 +13006,12 @@ on Intel Control-flow Enforcement Techno Emit extra code to check for buffer overflows, such as stack smashing attacks. This is done by adding a guard variable to functions with vulnerable objects. This includes functions that call @code{alloca}, and -functions with buffers larger than 8 bytes. The guards are initialized -when a function is entered and then checked when the function exits. -If a guard check fails, an error message is printed and the program exits. +functions with buffers larger than or equal to 8 bytes. The guards are +initialized when a function is entered and then checked when the function +exits. If a guard check fails, an error message is printed and the program +exits. Only variables that are actually allocated on the stack are +considered, optimized away variables or variables allocated in registers +don't count. @item -fstack-protector-all @opindex fstack-protector-all @@ -13018,7 +13021,9 @@ Like @option{-fstack-protector} except t @opindex fstack-protector-strong Like @option{-fstack-protector} but includes additional functions to be protected --- those that have local array definitions, or have -references to local frame addresses. +references to local frame addresses. Only variables that are actually +allocated on the stack are considered, optimized away variables or variables +allocated in registers don't count. @item -fstack-protector-explicit @opindex fstack-protector-explicit --- gcc/testsuite/gcc.target/i386/pr92825.c.jj 2019-12-09 14:49:38.691965021 +0100 +++ gcc/testsuite/gcc.target/i386/pr92825.c 2019-12-09 15:01:31.385176831 +0100 @@ -0,0 +1,15 @@ +/* PR middle-end/92825 */ +/* { dg-do compile { target fstack_protector } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ +/* { dg-final { scan-assembler-not "__stack_chk_fail" } } */ + +int +foo (int r, int g, int b) +{ + union U { int rgba; char p[4]; } u; + u.p[0] = r; + u.p[1] = g; + u.p[2] = b; + u.p[3] = -1; + return u.rgba; +}