| Message ID | CAEOtcjmFhFJ8A4r3nLq_odtFSctT51TOma0douMdBb27uKNnVA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 11/18/13 13:05, Marcos Díaz wrote: > Hi, > the attached patch adds a new attribute and option flag to control > when to do stack protection. > The new attribute (stack_protect) affects the behavior of gcc by > forcing the stack protection of the function marked with the attribute > if any of the options -fstack-protector, -fstack-protector-strong or > -fstack-protector-explicit(new) are set. > > If the new option (-fstack-protector-explicit) is set only those > functions with the attribute stack_protect will have stack protection. > > The stack re-ordering of the variables depends on the option set, > currently if flag -fstack-protector is set only char arrays are > reordered in the stack, whereas if flag -fstack-protector-strong or > -fstack-protector-explicit is set then char arrays and other arrays > are ordered first in the stack. > About this reordering of the non char arrays, shouldn't all to-be > protected functions have the full re-ordering? If not, for > completeness, I should make that new flag -fstack-protector-explicit > not to order the non-char arrays, and create a new -strong > counterpart, namely -fstack-protector-explicit-strong which does. > > Additionally, I think that the behavior of the flag > -fstack-protector-strong lacked the re-ordering of non char arrays > (named phase 2) so I added the reordering also for such flag. > Current tests pass after applying this patch, plus the tests specially added. > Please commit it for me if OK since I don't have write access. > > Changelog: > 2013-11-18 Marcos Diaz <marcos.diaz@tallertechnologies.com> [ ... ] Before doing any significant review on this work I have to ask, do you have a copyright assignment on file with the FSF and any necessary paperwork from your employer? Jeff
My employer is working on the signature of the papers. Could someone please do the review meanwhile? On Tue, Nov 19, 2013 at 3:00 AM, Jeff Law <law@redhat.com> wrote: > On 11/18/13 13:05, Marcos Díaz wrote: >> >> Hi, >> the attached patch adds a new attribute and option flag to control >> when to do stack protection. >> The new attribute (stack_protect) affects the behavior of gcc by >> forcing the stack protection of the function marked with the attribute >> if any of the options -fstack-protector, -fstack-protector-strong or >> -fstack-protector-explicit(new) are set. >> >> If the new option (-fstack-protector-explicit) is set only those >> functions with the attribute stack_protect will have stack protection. >> >> The stack re-ordering of the variables depends on the option set, >> currently if flag -fstack-protector is set only char arrays are >> reordered in the stack, whereas if flag -fstack-protector-strong or >> -fstack-protector-explicit is set then char arrays and other arrays >> are ordered first in the stack. >> About this reordering of the non char arrays, shouldn't all to-be >> protected functions have the full re-ordering? If not, for >> completeness, I should make that new flag -fstack-protector-explicit >> not to order the non-char arrays, and create a new -strong >> counterpart, namely -fstack-protector-explicit-strong which does. >> >> Additionally, I think that the behavior of the flag >> -fstack-protector-strong lacked the re-ordering of non char arrays >> (named phase 2) so I added the reordering also for such flag. >> Current tests pass after applying this patch, plus the tests specially >> added. >> Please commit it for me if OK since I don't have write access. >> >> Changelog: >> 2013-11-18 Marcos Diaz <marcos.diaz@tallertechnologies.com> > > [ ... ] > Before doing any significant review on this work I have to ask, do you have > a copyright assignment on file with the FSF and any necessary paperwork from > your employer? > > Jeff >
On 11/19/13 07:04, Marcos Díaz wrote: > My employer is working on the signature of the papers. Could someone > please do the review meanwhile? I'd prefer to wait until the assignment process is complete. If something were to happen and we can't use your code the review time would have been wasted (and such things have certainly happened in the past). Once the assignment is recorded, please ping this patch. Jeff
Well, finally I have the assignment, could you please review this patch? On Wed, Nov 20, 2013 at 4:13 PM, Jeff Law <law@redhat.com> wrote: > On 11/19/13 07:04, Marcos Díaz wrote: >> >> My employer is working on the signature of the papers. Could someone >> please do the review meanwhile? > > I'd prefer to wait until the assignment process is complete. If something > were to happen and we can't use your code the review time would have been > wasted (and such things have certainly happened in the past). > > Once the assignment is recorded, please ping this patch. > > Jeff >
On 03/19/14 08:06, Marcos Díaz wrote:
> Well, finally I have the assignment, could you please review this patch?
Thanks. I'll take a look once we open up stage1 development again
(should be soon as 4.9 is getting close to being ready).
jeff
On Wed, Mar 19, 2014 at 11:09 AM, Jeff Law <law@redhat.com> wrote: > On 03/19/14 08:06, Marcos Díaz wrote: >> >> Well, finally I have the assignment, could you please review this patch? > > Thanks. I'll take a look once we open up stage1 development again (should > be soon as 4.9 is getting close to being ready). > > jeff > Ping!
On 03/19/14 08:06, Marcos Díaz wrote:
> Well, finally I have the assignment, could you please review this patch?
Thanks.
My first thought was that if we've marked the function with an explicit
static protector attribute, then it ought to be protected regardless of
any flags. Is there some reason to require the -fstack-protect-explicit?
The patch itself is relatively simple and I don't see anything that
looks terribly wrong at first glance. I think we just need to make sure
we're on the same page WRT needing the -fstack-protect-explicit flag.
jeff
On Tue, Jul 1, 2014 at 2:25 PM, Jeff Law <law@redhat.com> wrote: > On 03/19/14 08:06, Marcos Díaz wrote: >> >> Well, finally I have the assignment, could you please review this patch? > > Thanks. > > My first thought was that if we've marked the function with an explicit > static protector attribute, then it ought to be protected regardless of any > flags. Is there some reason to require the -fstack-protect-explicit? They can work separately, since the logic is: if NOT stack-protect-explicit a function can be protected by the current logic OR it has the attribute (a function may be not automatically protected with the current logic) ELSE // stack-protect-explicit only functions marked with the attribute will be protected. IOW, when no stack-protect-explicit, the functions may not be protected due to current logic, so the attribute acts as an override to request protection. > > The patch itself is relatively simple and I don't see anything that looks > terribly wrong at first glance. I think we just need to make sure we're on > the same page WRT needing the -fstack-protect-explicit flag. > > jeff > >
On Tue, Jul 1, 2014 at 6:34 PM, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > On Tue, Jul 1, 2014 at 2:25 PM, Jeff Law <law@redhat.com> wrote: >> On 03/19/14 08:06, Marcos Díaz wrote: >>> >>> Well, finally I have the assignment, could you please review this patch? >> >> Thanks. >> >> My first thought was that if we've marked the function with an explicit >> static protector attribute, then it ought to be protected regardless of any >> flags. Is there some reason to require the -fstack-protect-explicit? > > They can work separately, since the logic is: > > if NOT stack-protect-explicit > a function can be protected by the current logic OR it has the attribute > (a function may be not automatically protected with the current logic) > ELSE // stack-protect-explicit > only functions marked with the attribute will be protected. > If there isn't any stack-protect flag (strong, common or explicit) the attribute has no effect > IOW, when no stack-protect-explicit, the functions may not be > protected due to current logic, so the attribute acts as an override > to request protection. > >> >> The patch itself is relatively simple and I don't see anything that looks >> terribly wrong at first glance. I think we just need to make sure we're on >> the same page WRT needing the -fstack-protect-explicit flag. >> >> jeff >> >> > > > > -- > > Daniel F. Gutson > Chief Engineering Officer, SPD > > > San Lorenzo 47, 3rd Floor, Office 5 > > Córdoba, Argentina > > > Phone: +54 351 4217888 / +54 351 4218211 > > Skype: dgutson
On Wed, Jul 2, 2014 at 2:58 PM, Marcos Díaz <marcos.diaz@tallertechnologies.com> wrote: > On Tue, Jul 1, 2014 at 6:34 PM, Daniel Gutson > <daniel.gutson@tallertechnologies.com> wrote: >> On Tue, Jul 1, 2014 at 2:25 PM, Jeff Law <law@redhat.com> wrote: >>> On 03/19/14 08:06, Marcos Díaz wrote: >>>> >>>> Well, finally I have the assignment, could you please review this patch? >>> >>> Thanks. >>> >>> My first thought was that if we've marked the function with an explicit >>> static protector attribute, then it ought to be protected regardless of any >>> flags. Is there some reason to require the -fstack-protect-explicit? >> >> They can work separately, since the logic is: >> >> if NOT stack-protect-explicit >> a function can be protected by the current logic OR it has the attribute >> (a function may be not automatically protected with the current logic) >> ELSE // stack-protect-explicit >> only functions marked with the attribute will be protected. >> > If there isn't any stack-protect flag (strong, common or explicit) the > attribute has no effect >> IOW, when no stack-protect-explicit, the functions may not be >> protected due to current logic, so the attribute acts as an override >> to request protection. >> >>> >>> The patch itself is relatively simple and I don't see anything that looks >>> terribly wrong at first glance. I think we just need to make sure we're on >>> the same page WRT needing the -fstack-protect-explicit flag. >>> >>> jeff >>> >>> >> >> >> >> -- >> >> Daniel F. Gutson >> Chief Engineering Officer, SPD >> >> >> San Lorenzo 47, 3rd Floor, Office 5 >> >> Córdoba, Argentina >> >> >> Phone: +54 351 4217888 / +54 351 4218211 >> >> Skype: dgutson > > > > -- > ______________________________ > > > Marcos Díaz > > Software Engineer > > > San Lorenzo 47, 3rd Floor, Office 5 > > Córdoba, Argentina > > > Phone: +54 351 4217888 / +54 351 4218211/ +54 351 7617452 > > Skype: markdiaz22 ping
On 07/01/14 15:34, Daniel Gutson wrote: > On Tue, Jul 1, 2014 at 2:25 PM, Jeff Law <law@redhat.com> wrote: >> On 03/19/14 08:06, Marcos Díaz wrote: >>> >>> Well, finally I have the assignment, could you please review this patch? >> >> Thanks. >> >> My first thought was that if we've marked the function with an explicit >> static protector attribute, then it ought to be protected regardless of any >> flags. Is there some reason to require the -fstack-protect-explicit? > > They can work separately, since the logic is: > > if NOT stack-protect-explicit > a function can be protected by the current logic OR it has the attribute > (a function may be not automatically protected with the current logic) > ELSE // stack-protect-explicit > only functions marked with the attribute will be protected. > > IOW, when no stack-protect-explicit, the functions may not be > protected due to current logic, so the attribute acts as an override > to request protection. Sorry this took so long. I fixed a variety of whitespace errors, wrote a better ChangeLog, re-bootstrapped and regression tested the patch (given the long delay, I felt it was the least I could do). Approved and installed. Sorry for the terribly long delay. jeff
Index: gcc/testsuite/gcc.dg/stackprotectexplicit1.c =================================================================== --- gcc/testsuite/gcc.dg/stackprotectexplicit1.c (revision 0) +++ gcc/testsuite/gcc.dg/stackprotectexplicit1.c (revision 0) @@ -0,0 +1,23 @@ +/* { dg-do run { target native } } */ +/* { dg-options "-fstack-protector-explicit" } */ +/* { dg-require-effective-target fstack_protector } */ + +#include <stdlib.h> + +void +__stack_chk_fail (void) +{ + exit (0); /* pass */ +} + +int __attribute__((stack_protect)) main () +{ + int i; + char foo[255]; + + // smash stack + for (i = 0; i <= 400; i++) + foo[i] = 42; + + return 1; /* fail */ +} Index: gcc/testsuite/g++.dg/stackprotectexplicit2.C =================================================================== --- gcc/testsuite/g++.dg/stackprotectexplicit2.C (revision 0) +++ gcc/testsuite/g++.dg/stackprotectexplicit2.C (revision 0) @@ -0,0 +1,27 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-explicit" } */ + +int A() +{ + int A[23]; + char b[22]; +} + +int __attribute__((stack_protect)) B() +{ + int a; + int b; + return a+b; +} + +int __attribute__((stack_protect)) c() +{ + int a; + char b[34]; + return 0; +} + + +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */ \ No newline at end of file Index: gcc/common.opt =================================================================== --- gcc/common.opt (revision 204949) +++ gcc/common.opt (working copy) @@ -1958,6 +1958,10 @@ fstack-protector-strong Common Report RejectNegative Var(flag_stack_protect, 3) Use a smart stack protection method for certain functions +fstack-protector-explicit +Common Report RejectNegative Var(flag_stack_protect, 4) +Use stack protection method only for functions with the stack_protect attribute + fstack-usage Common RejectNegative Var(flag_stack_usage) Output stack usage information on a per-function basis Index: gcc/c-family/c-cppbuiltin.c =================================================================== --- gcc/c-family/c-cppbuiltin.c (revision 204949) +++ gcc/c-family/c-cppbuiltin.c (working copy) @@ -984,6 +984,8 @@ c_cpp_builtins (cpp_reader *pfile) /* Make the choice of the stack protector runtime visible to source code. The macro names and values here were chosen for compatibility with an earlier implementation, i.e. ProPolice. */ + if (flag_stack_protect == 4) + cpp_define (pfile, "__SSP_EXPLICIT__=4"); if (flag_stack_protect == 3) cpp_define (pfile, "__SSP_STRONG__=3"); if (flag_stack_protect == 2) Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 204949) +++ gcc/c-family/c-common.c (working copy) @@ -314,6 +314,7 @@ static tree handle_no_address_safety_ana int, bool *); static tree handle_no_sanitize_undefined_attribute (tree *, tree, tree, int, bool *); +static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *); static tree handle_noinline_attribute (tree *, tree, tree, int, bool *); static tree handle_noclone_attribute (tree *, tree, tree, int, bool *); static tree handle_leaf_attribute (tree *, tree, tree, int, bool *); @@ -629,6 +630,8 @@ const struct attribute_spec c_common_att handle_noreturn_attribute, false }, { "volatile", 0, 0, true, false, false, handle_noreturn_attribute, false }, + { "stack_protect", 0, 0, true, false, false, + handle_stack_protect_attribute, false }, { "noinline", 0, 0, true, false, false, handle_noinline_attribute, false }, { "noclone", 0, 0, true, false, false, @@ -6624,6 +6627,25 @@ handle_no_sanitize_undefined_attribute ( return NULL_TREE; } + +/* Handle a "stack_protect" attribute; arguments as in + struct attribute_spec.handler. */ +static tree +handle_stack_protect_attribute (tree *node, tree name, tree, int, + bool *no_add_attrs) +{ + if (TREE_CODE (*node) != FUNCTION_DECL) + { + warning (OPT_Wattributes, "%qE attribute ignored", name); + *no_add_attrs = true; + } + else + DECL_ATTRIBUTES (*node) + = tree_cons (get_identifier ("stack_protect"), + NULL_TREE, DECL_ATTRIBUTES (*node)); + + return NULL_TREE; +} /* Handle a "noinline" attribute; arguments as in struct attribute_spec.handler. */ Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 204949) +++ gcc/cfgexpand.c (working copy) @@ -1330,7 +1330,8 @@ clear_tree_used (tree block) enum { SPCT_FLAG_DEFAULT = 1, SPCT_FLAG_ALL = 2, - SPCT_FLAG_STRONG = 3 + SPCT_FLAG_STRONG = 3, + SPCT_FLAG_EXPLICIT = 4 }; /* Examine TYPE and determine a bit mask of the following features. */ @@ -1403,7 +1404,9 @@ stack_protect_decl_phase (tree decl) has_short_buffer = true; if (flag_stack_protect == SPCT_FLAG_ALL - || flag_stack_protect == SPCT_FLAG_STRONG) + || flag_stack_protect == SPCT_FLAG_STRONG + || (flag_stack_protect == SPCT_FLAG_EXPLICIT + && lookup_attribute("stack_protect",DECL_ATTRIBUTES(current_function_decl)))) { if ((bits & (SPCT_HAS_SMALL_CHAR_ARRAY | SPCT_HAS_LARGE_CHAR_ARRAY)) && !(bits & SPCT_HAS_AGGREGATE)) @@ -1743,7 +1746,10 @@ expand_used_vars (void) /* If stack protection is enabled, we don't share space between vulnerable data and non-vulnerable data. */ - if (flag_stack_protect) + if (flag_stack_protect != 0 + && (flag_stack_protect != SPCT_FLAG_EXPLICIT + || (flag_stack_protect == SPCT_FLAG_EXPLICIT + && lookup_attribute("stack_protect",DECL_ATTRIBUTES(current_function_decl))))) add_stack_protection_conflicts (); /* Now that we have collected all stack variables, and have computed a @@ -1761,15 +1767,22 @@ 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 (); break; + case SPCT_FLAG_EXPLICIT: + if (lookup_attribute("stack_protect", + DECL_ATTRIBUTES(current_function_decl))) + create_stack_guard(); + break; default: ; } @@ -1793,8 +1806,11 @@ expand_used_vars (void) expand_stack_vars (stack_protect_decl_phase_1, &data); /* Phase 2 contains other kinds of arrays. */ - if (flag_stack_protect == 2) - expand_stack_vars (stack_protect_decl_phase_2, &data); + if (flag_stack_protect == SPCT_FLAG_ALL + || flag_stack_protect == SPCT_FLAG_STRONG + || (flag_stack_protect == SPCT_FLAG_EXPLICIT + && lookup_attribute("stack_protect",DECL_ATTRIBUTES(current_function_decl)))) + expand_stack_vars (stack_protect_decl_phase_2, &data); } if (flag_sanitize & SANITIZE_ADDRESS) Index: gcc/doc/cpp.texi =================================================================== --- gcc/doc/cpp.texi (revision 204949) +++ gcc/doc/cpp.texi (working copy) @@ -2353,6 +2353,10 @@ in use. This macro is defined, with value 3, when @option{-fstack-protector-strong} is in use. +@item __SSP_EXPLICIT__ +This macro is defined, with value 4, when @option{-fstack-protector-explicit} is +in use. + @item __SANITIZE_ADDRESS__ This macro is defined, with value 1, when @option{-fsanitize=address} is in use. Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 204949) +++ gcc/doc/invoke.texi (working copy) @@ -408,7 +408,8 @@ 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 -fstack-protector-strong -fstrict-aliasing @gol +-fstack-protector-all -fstack-protector-strong -fstack-protector-explicit @gol +-fstrict-aliasing @gol -fstrict-overflow -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 @@ -9057,6 +9058,11 @@ Like @option{-fstack-protector} but incl be protected --- those that have local array definitions, or have references to local frame addresses. +@item -fstack-protector-explicit +@opindex fstack-protector-explicit +Like @option{-fstack-protector} but only protects those functions which +have the @code{stack_protect} attribute + @item -fsection-anchors @opindex fsection-anchors Try to reduce the number of symbolic address calculations by using Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 204949) +++ gcc/doc/extend.texi (working copy) @@ -2167,7 +2167,7 @@ attributes are currently defined for fun @code{returns_nonnull}, @code{gnu_inline}, @code{externally_visible}, @code{hot}, @code{cold}, @code{artificial}, @code{no_sanitize_address}, @code{no_address_safety_analysis}, -@code{no_sanitize_undefined}, @code{bnd_legacy}, +@code{no_sanitize_undefined}, @code{bnd_legacy}, @code{stack_protect} @code{error} and @code{warning}. Several other attributes are defined for functions on particular target systems. Other attributes, including @code{section} are @@ -3319,6 +3319,12 @@ prologue which decides whether to split @code{no_split_stack} attribute do not have that prologue, and thus may run with only a small amount of stack space available. +@item stack_protect +@cindex @code{stack_protect} function attribute +This function attribute make a stack protection of the function if +flags @option{fstack-protector} or @option{fstack-protector-strong} +or @option{fstack-protector-explicit} are set. + @item noinline @cindex @code{noinline} function attribute This function attribute prevents a function from being considered for
Hi, the attached patch adds a new attribute and option flag to control when to do stack protection. The new attribute (stack_protect) affects the behavior of gcc by forcing the stack protection of the function marked with the attribute if any of the options -fstack-protector, -fstack-protector-strong or -fstack-protector-explicit(new) are set. If the new option (-fstack-protector-explicit) is set only those functions with the attribute stack_protect will have stack protection. The stack re-ordering of the variables depends on the option set, currently if flag -fstack-protector is set only char arrays are reordered in the stack, whereas if flag -fstack-protector-strong or -fstack-protector-explicit is set then char arrays and other arrays are ordered first in the stack. About this reordering of the non char arrays, shouldn't all to-be protected functions have the full re-ordering? If not, for completeness, I should make that new flag -fstack-protector-explicit not to order the non-char arrays, and create a new -strong counterpart, namely -fstack-protector-explicit-strong which does. Additionally, I think that the behavior of the flag -fstack-protector-strong lacked the re-ordering of non char arrays (named phase 2) so I added the reordering also for such flag. Current tests pass after applying this patch, plus the tests specially added. Please commit it for me if OK since I don't have write access. Changelog: 2013-11-18 Marcos Diaz <marcos.diaz@tallertechnologies.com> gcc/ * c-family/c-cppbuiltin.c (c_cpp_builtins): New cpp define __SSP_EXPLICIT__ for the new option fstack-protector_explicit * c-family/c-common.c: Added new attribute stack-protect and respective handler * common.opt: Added new option -fstack-protector-explicit * cfgexpand.c: Added new enum SPCT_FLAG_EXPLICIT (stack_protect_decl_phase): Added new condition in the look of conflictive variables for the new attribute and option. (expand_used_vars): Added new condition in order to put stack protection in functions with the stack protect attribute. gcc/doc/ * cpp.texi: New description for the new cpp_define __SSP_EXPLICIT__ * extend.texi: Added description for the new stack_protect function attribute. * invoke.texi: description for the new -fstack-protector-strong option gcc/testsuite/ * gcc.dg/stackprotectexplicit1.c: New test file for the flag and the attribute * g++.dg/stackprotectexplicit2.C: New test file for the flag and the attribute