From patchwork Thu Jun 14 23:09:58 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Han Shen X-Patchwork-Id: 165025 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 93A8BB706C for ; Fri, 15 Jun 2012 09:10:33 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1340320234; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: MIME-Version:Received:Received:Date:Message-ID:Subject:From:To: Cc:Content-Type:Content-Transfer-Encoding:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=BBnRIcv5SKR0d9oHbSIgeALda3I=; b=b4MKNKjZd1nR2iVut6dmL/GfwGft8KBQZysFGjchbgUE1HmFlYJefPqq9VNup0 M/9P1Oyj6l53WGiFsyn3NLNBT6JNRFSveiVfdABE1sJwUNi1ZbrRY1lAFfiOJ9Td xFK8IsEZkD3yrpwwuvPIwD+7yDeplK5tbWUE0uI2e9QgM= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Google-DKIM-Signature:Received:MIME-Version:Received:Received:Date:Message-ID:Subject:From:To:Cc:Content-Type:Content-Transfer-Encoding:X-System-Of-Record:X-Gm-Message-State:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=C1UEWkz12NDBBUAam/LI/0L+XAyUmJUkEDaCq/y3oNZmjSrXAuTjanzVPnUW+d o26QcH46Vf+SwhFkB06gRLknmqLw2Xl/EMlDfi35N34h8+1RBHgHejokKFOdcUXg sZDPYSBCjwVUaMoBMCtMDPjNHaFUtqZbtnlQzbz8MQ0o8=; Received: (qmail 27772 invoked by alias); 14 Jun 2012 23:10:24 -0000 Received: (qmail 27747 invoked by uid 22791); 14 Jun 2012 23:10:19 -0000 X-SWARE-Spam-Status: No, hits=-4.0 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, KHOP_RCVD_TRUST, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, TW_CP, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-vc0-f175.google.com (HELO mail-vc0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 14 Jun 2012 23:09:59 +0000 Received: by vcbfl15 with SMTP id fl15so1524391vcb.20 for ; Thu, 14 Jun 2012 16:09:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding:x-system-of-record:x-gm-message-state; bh=OR2QxOZIyi0pNMo7wuZrvDulfWWY915oQxatx6JrrRs=; b=Qgo0fSNOpwkX+m2og6SViJYcGXQHkHST2kR6UG4iLcStlUdoZY6dRk2SGDDFk/mJJD e0MDP3+9Ic8CzLqYH+LAMZ02knF6o4fuqyentZNgPHoLEk6K4atKsVPpSTAekylMXx4e UoaV8xR32Vz9iwprF/p74heKspPZk1ZDF9BO1vP4aLLtEjCuZO2QjO5viyaFfr9iKvDs xMNsQe6UnPflXNOqtefmb9aNgIQkh3CTRiAT0utIBM69vRKhUG62EoXCIhEbNNwh9fGm uJyy1PQi38vEdxjPGTelbo83LNU6DwNKITurKx0SFoYQmN6iUsm+Mtp8dmIXIZ6/w2HD BU2A== Received: by 10.52.99.227 with SMTP id et3mr1658260vdb.110.1339715398455; Thu, 14 Jun 2012 16:09:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.99.227 with SMTP id et3mr1658247vdb.110.1339715398257; Thu, 14 Jun 2012 16:09:58 -0700 (PDT) Received: by 10.52.180.167 with HTTP; Thu, 14 Jun 2012 16:09:58 -0700 (PDT) Date: Thu, 14 Jun 2012 16:09:58 -0700 Message-ID: Subject: [PATCH] Add a new option "-fstack-protector-strong" (patch / doc inside) From: =?UTF-8?B?SGFuIFNoZW4o5rKI5ra1KQ==?= To: gcc-patches@gcc.gnu.org Cc: Diego Novillo , Jing Yu , Kees Cook , Ahmad Sharif , David Li , Rong Xu X-System-Of-Record: true X-Gm-Message-State: ALoCoQl3DBVYX7BkpOtfkEJ29FSab9a6S9wjHuPz6ppaBL5pLzSmDDSr60VdVfAGqS4xwfHCbOp7bBX3K8Gd+GgHrLxZb+aDfSYPF7F9oiEv+ZDG7qijczMuAO2s1tElFbo0Fyf434BwfJskiUlR673UPbrHzjiY1A== X-IsSubscribed: yes 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 Hi, This is to port the patch from google/main to trunk, which provides a new stack protection option - "fstack-protector-strong". Previous review for google trunk is here - http://codereview.appspot.com/5461043 Status - it has been used in google/main for 2 quarters, building the whole chromiumos with no securiy degradation. Benefit - gain big performance while sacrificing little security (for scenarios using -fstack-protector-all) Background - some times stack-protector is too-simple while stack-protector-all over-kills, for example, to build one of our core systems, we forcibly add "-fstack-protector-all" to all compile commands, which brings big performance penalty (due to extra stack guard/check insns on function prologue and epilogue) on both atom and arm. To use "-fstack-protector" is just regarded as not secure enough (only "protects" <2% functions) by the system secure team. So I'd like to add the option "-fstack-protector-strong", that hits the balance between "-fstack-protector" and "-fstack-protector-all". Tested - building chromiumos from scratch. Changelog - gcc/ChangeLog: * cfgexpand.c (expand_used_vars): Add logic handling stack-protector-strong. (record_or_union_type_has_array_p): New, tests if a record or union type contains an array. * common.opt (fstack-protector-all): New option. * doc/invoke.texi: Added reference to "-fstack-protector-strong". gcc/testsuite/ChangeLog * gcc.dg/fstack-protector-strong.c: New. * g++.dg/fstack-protector-strong.C: New. Patch - Design doc - Proposal to add compiler option “-fstack-protector-strong” 1. Current stack protection options Currently, gcc only has 2 options regarding stack protector against SSA (stack smashing attack) - stack-protector Add stack protection to functions that have “alloca” or have a (signed or unsigned) char array with size > 8 (SSP_BUFFER_SIZE) - stack-protector-all Add stack protection to ALL functions. 2. Problems with current stack protection options The stack-protector option is over-simplified, which ignores pointer cast, address computation, while the stack-protector-all is over-killing, using this option brings too much performance overhead to both arm- and atom- chrome browser. 3. Propose a new stack protector option - stack-protector-strong This option tries to hit the balance between an over-simplified version and an over-killing protection schema. It chooses more functions to be protected than “stack-protector”, it is a superset of “stack-protector”, for functions not chosen by “stack-protector”, “stack-protector-strong” will apply protection if any of the following conditions meets. - if any of its local variable’s address is taken,as part of the RHS of an assignment - or if any of its local variable’s address is taken as part of a function argument. - or if it has an array, regardless of array type or length - or if it has a struct/union which contains an array, regardless of array type or length. - or if function has register local variables 4. Possible performance gain for atom and tegra board Replacing “stack-protector-all” with “stack-protector-strong” sees a good performance gain. 5. Ideas behind this implementation: The basic idea is that any stack smashing attack needs a frame address to perform the attack. The frame address of function F can be from one of the following: - (A) an address taking operator (&) on any local variables of F - (B) any address computation based on (A) - (C) any address casting operation from either (A) or (B) - (D) the name of a local array of F - (E) the address from calling “alloca” Function F is said to be vulnerable if its frame address is exposed via (A) ~ (E). “stack-protector-strong” just protects these vulnerable functions. 6. Stack smashing attack illustrated If in function F, we have pointer P, which points to function G's stack, you can only attack frames of function G and functions calling G, for example X and XX. To add guard0 to frame G protects everything above guard0 being attacked from P". (Note, v0 and v1 are still attack-able, this won't be fixed even if you add guard to all frames.) Suppose now you have Q, which points to XX's frame, guard0 will not prevent attacking from Q, so we have to add guard1. To summarize, we just need to add guard to functions whose frame address is exposed(escaped) - either by address taken operator (&), or by passing address (or array) around via function call. (See picture below) (I cannot upload pictures here, to see the origin picture - refer here - https://docs.google.com/a/google.com/document/d/1xXBH6rRZue4f296vGt9YQcuLVQHeE516stHwt8M9xyU/edit?hl=en_US) Patch also uploaded to http://codereview.appspot.com/6303078/ Regards, -Han diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 8a31a9f..0911b6c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1511,15 +1511,39 @@ estimated_stack_frame_size (struct cgraph_node *node) return 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)) + return record_or_union_type_has_array_p (field_type); + if (TREE_CODE (field_type) == ARRAY_TYPE) + return 1; + } + } + return 0; +} + /* Expand all variables used in the function. */ static void expand_used_vars (void) { tree var, outer_block = DECL_INITIAL (current_function_decl); + referenced_var_iterator rvi; VEC(tree,heap) *maybe_local_decls = NULL; unsigned i; unsigned len; + int gen_stack_protect_signal = 0; /* Compute the phase of the stack frame for this function. */ { @@ -1552,6 +1576,23 @@ expand_used_vars (void) } } + FOR_EACH_REFERENCED_VAR (cfun, var, rvi) + if (!is_global_var (var)) + { + tree var_type = TREE_TYPE (var); + /* Examine local referenced variables that have their addresses taken, + contain an array, or are arrays. */ + if (TREE_CODE (var) == VAR_DECL + && (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)))) + { + ++gen_stack_protect_signal; + break; + } + } + /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -1642,11 +1683,18 @@ expand_used_vars (void) dump_stack_var_partition (); } - /* There are several conditions under which we should create a - stack guard: protect-all, alloca used, protected decls present. */ - if (flag_stack_protect == 2 - || (flag_stack_protect - && (cfun->calls_alloca || has_protected_decls))) + /* Create stack guard, if + a) "-fstack-protector-all" - always; + b) "-fstack-protector-strong" - if there are arrays, memory + references to local variables, alloca used, or protected decls present; + c) "-fstack-protector" - if alloca used, or protected decls present */ + if (flag_stack_protect == 3 /* -fstack-protector-all */ + || (flag_stack_protect == 2 /* -fstack-protector-strong */ + && (gen_stack_protect_signal || cfun->calls_alloca + || has_protected_decls)) + || (flag_stack_protect == 1 /* -fstack-protector */ + && (cfun->calls_alloca + || has_protected_decls))) create_stack_guard (); /* Assign rtl to each variable based on these partitions. */ diff --git a/gcc/common.opt b/gcc/common.opt index 5b1b4d8..44447f6 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1846,8 +1846,12 @@ fstack-protector Common Report Var(flag_stack_protect, 1) Use propolice as a stack protection method -fstack-protector-all +fstack-protector-strong Common Report RejectNegative Var(flag_stack_protect, 2) +Use a smart stack protection method for certain functions + +fstack-protector-all +Common Report RejectNegative Var(flag_stack_protect, 3) Use a stack protection method for every function fstack-usage diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 9fa0085..44f2034 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -403,7 +403,7 @@ Objective-C and Objective-C++ Dialects}. -fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol -fshrink-wrap -fsignaling-nans -fsingle-precision-constant @gol -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol --fstack-protector-all -fstrict-aliasing -fstrict-overflow @gol +-fstack-protector-all -fstack-protector-strong -fstrict-aliasing -fstrict-overflow @gol -fthread-jumps -ftracer -ftree-bit-ccp @gol -ftree-builtin-call-dce -ftree-ccp -ftree-ch @gol -ftree-coalesce-inline-vars -ftree-coalesce-vars -ftree-copy-prop @gol @@ -8564,6 +8564,12 @@ If a guard check fails, an error message is printed and the program exits. @opindex fstack-protector-all Like @option{-fstack-protector} except that all functions are protected. +@item -fstack-protector-strong +@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. + @item -fsection-anchors @opindex fsection-anchors Try to reduce the number of symbolic address calculations by using diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C b/gcc/testsuite/g++.dg/fstack-protector-strong.C new file mode 100644 index 0000000..a4f0f81 --- /dev/null +++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C @@ -0,0 +1,35 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +class A +{ +public: + A() {} + ~A() {} + void method(); + int state; +}; + +/* Frame address exposed to A::method via "this". */ +int +foo1 () +{ + A a; + a.method (); + return a.state; +} + +/* Possible destroying foo2's stack via &a. */ +int +global_func (A& a); + +/* Frame address exposed to global_func. */ +int foo2 () +{ + A a; + return global_func (a); +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */ diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c b/gcc/testsuite/gcc.dg/fstack-protector-strong.c new file mode 100644 index 0000000..5a5cf98 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c @@ -0,0 +1,135 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +#include +#include + +extern int g0; +extern int* pg0; +int +goo (int *); +int +hoo (int); + +/* Function frame address escaped function call. */ +int +foo1 () +{ + int i; + return goo (&i); +} + +struct ArrayStruct +{ + int a; + int array[10]; +}; + +struct AA +{ + int b; + struct ArrayStruct as; +}; + +/* Function frame contains array. */ +int +foo2 () +{ + struct AA aa; + int i; + for (i = 0; i < 10; ++i) + { + aa.as.array[i] = i * (i-1) + i / 2; + } + return aa.as.array[5]; +} + +/* Address computation based on a function frame address. */ +int +foo3 () +{ + int a; + int *p; + p = &a + 5; + return goo (p); +} + +/* Address cast based on a function frame address. */ +int +foo4 () +{ + int a; + return goo (g0 << 2 ? (int *)(3 * (long)(void *)(&a)) : 0); +} + +/* Address cast based on a local array. */ +int +foo5 () +{ + short array[10]; + return goo ((int *)(array + 5)); +} + +struct BB +{ + int one; + int two; + int three; +}; + +/* Address computaton based on a function frame address.*/ +int +foo6 () +{ + struct BB bb; + return goo (&bb.one + sizeof(int)); +} + +/* Function frame address escaped via global variable. */ +int +foo7 () +{ + int a; + pg0 = &a; + goo (pg0); + return *pg0; +} + +/* Check that this covers -fstack-protector. */ +int +foo8 () +{ + char base[100]; + memcpy ((void *)base, (const void *)pg0, 105); + return (int)(base[32]); +} + +/* Check that this covers -fstack-protector. */ +int +foo9 () +{ + char* p = alloca (100); + return goo ((int *)(p + 50)); +} + +int +global2 (struct BB* pbb); + +/* Address taken on struct. */ +int +foo10 () +{ + struct BB bb; + int i; + bb.one = global2 (&bb); + for (i = 0; i < 10; ++i) + { + bb.two = bb.one + bb.two; + bb.three = bb.one + bb.two + bb.three; + } + return bb.three; +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */