Hi Florian, thanks for the review!
On Tue, Apr 16, 2013 at 6:32 AM, Florian Weimer <fwei...@redhat.com> wrote:
Please include the proposed changelog entries.
Done.
+ if (flag_stack_protect == 3)
+ cpp_define (pfile, "__SSP_STRONG__=3");
if (flag_stack_protect == 2)
cpp_define (pfile, "__SSP_ALL__=2");
3 and 2 should be replaced by SPCT_FLAG_STRONG and SPCT_FLAG_ALL.
I define these SPCT_FLAG_XXX in cfgexpand.c locally, so they are not
visible to c-cppbuiltin.c, do you suggest define these inside
c-cppbuiltin.c also?
+/* 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;
+}
Indentation is off (unless both mail clients I tried are clobbering your
patch). I think the GNU coding style prohibits the braces around the
single-statement body of the outer 'for".
Done with indentation properly on and removed the braces. (GMail
composing window drops all the tabs when pasting... I have to use
Thunderbird to paste the patch. Hope it is right this time)
+
/* Expand all variables used in the function. */
static rtx
@@ -1525,6 +1553,7 @@ expand_used_vars (void)
struct pointer_map_t *ssa_name_decls;
unsigned i;
unsigned len;
+ int gen_stack_protect_signal = 0;
This should be a bool variable, initialized with false, and which is
assigned true below.
Done
/* Compute the phase of the stack frame for this function. */
{
@@ -1576,6 +1605,23 @@ expand_used_vars (void)
}
pointer_map_destroy (ssa_name_decls);
+ FOR_EACH_LOCAL_DECL (cfun, i, var)
+ 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;
+ }
+ }
+
Indentation again. This analysis needs to be performed in SPCT_FLAG_STRONG
mode only, it can be skipped in the other modes. It might make sense to run
it only if the other conditions checked below do not hold.
Wrapped it with testing if SPCT_FLAG_STRONG is mode on.
/* At this point all variables on the local_decls with TREE_USED
set are not associated with any block scope. Lay them out. */
@@ -1662,11 +1708,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 == SPCT_FLAG_ALL
+ || (flag_stack_protect == SPCT_FLAG_STRONG
+ && (gen_stack_protect_signal || cfun->calls_alloca
+ || has_protected_decls))
+ || (flag_stack_protect == SPCT_FLAG_DEFAULT
+ && (cfun->calls_alloca
+ || has_protected_decls)))
create_stack_guard ();
Can you make the conditional more similar to the comment, perhaps using a
switch statement on the value of the flag_stack_protect variable? That's
going to be much easier to read.
Re-coded. Now using 'switch-case'.
+@item -fstack-protector-strong
+@opindex fstack-protector-strong
+Like @option{-fstack-protector-strong} but includes additional functions
to be
+protected - those that have local array definitions, or have
references to local
+frame addresses.
"Like @option{-fstack-protector}", I presume. Replace " - " with "---".
Does "reference local frame addresses" mean "take addresses of local
variables" (at least for C/C++)?
Done. And yes.
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fstack-protector-strong" } */
Do we have a better target selection for the test cases? I think at least
rs6000 and s390x should support this as well.
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).
What about struct-returning functions? Internally, an address is passed to
the called function. Would they trigger this? What about the this pointer
in C++ code?
Yes for 'this' pointer. 'this' pointer of a local class instance is
regarded as a reference to stack address, thus it is protected. For
example -
int
foo1 ()
{
A a;
a.method (); // a.method() exposes 'this', so this function is
protected.
return a.state;
}
No for 'struct-returning' functions. But I regard this not an issue ---
at the programming level, there is no way to get one's hand on the
address of a returned structure ---
struct Node foo();
struct Node *p = &foo(); // compiler error - lvalue required as
unary '&' operand.
If write this way -
struct Node p = foo();
struct Node *q = &p;
The protection would be triggered.
--
Florian Weimer / Red Hat Product Security Team
ChangeLog and patch below --
gcc/ChangeLog
2013-04-16 Han Shen <shen...@google.com>
* cfgexpand.c (record_or_union_type_has_array_p): Helper function
to check if a record or union contains an array field.
(expand_used_vars): Add logic handling '-fstack-protector-strong'.
* common.opt (fstack-protector-all): New option.
* doc/cpp.texi (__SSP_STRONG__): New builtin "__SSP_STRONG__".
* doc/invoke.texi (Optimization Options): Document
"-fstack-protector-strong".
* gcc.c (LINK_SSP_SPEC): Add 'fstack-protector-strong'.
gcc/c-family/ChangeLog
2013-04-16 Han Shen <shen...@google.com>
* c-cppbuiltin.c (c_cpp_builtins): Added "__SSP_STRONG__=3".
gcc/testsuite/ChangeLog
2013-04-16 Han Shen <shen...@google.com>
Test cases for '-fstack-protector-strong'.
* gcc.dg/fstack-protector-strong.c: New.
* g++.dg/fstack-protector-strong.C: New.
diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index 3e210d9..0059626 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -888,6 +888,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 == 3)
+ cpp_define (pfile, "__SSP_STRONG__=3");
if (flag_stack_protect == 2)
cpp_define (pfile, "__SSP_ALL__=2");
else if (flag_stack_protect == 1)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a651d8c..b370cdb 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1291,6 +1291,10 @@ clear_tree_used (tree block)
clear_tree_used (t);
}
+#define SPCT_FLAG_ALL 2
+#define SPCT_FLAG_DEFAULT 1
+#define SPCT_FLAG_STRONG 3
+
/* Examine TYPE and determine a bit mask of the following features. */
#define SPCT_HAS_LARGE_CHAR_ARRAY 1
@@ -1360,7 +1364,8 @@ stack_protect_decl_phase (tree decl)
if (bits & SPCT_HAS_SMALL_CHAR_ARRAY)
has_short_buffer = true;
- if (flag_stack_protect == 2)
+ if (flag_stack_protect == SPCT_FLAG_ALL ||
+ flag_stack_protect == SPCT_FLAG_STRONG)
{
if ((bits & (SPCT_HAS_SMALL_CHAR_ARRAY | SPCT_HAS_LARGE_CHAR_ARRAY))
&& !(bits & SPCT_HAS_AGGREGATE))
@@ -1514,6 +1519,27 @@ 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)
+ && record_or_union_type_has_array_p (field_type))
+ return 1;
+ if (TREE_CODE (field_type) == ARRAY_TYPE)
+ return 1;
+ }
+ return 0;
+}
+
/* Expand all variables used in the function. */
static rtx
@@ -1525,6 +1551,7 @@ expand_used_vars (void)
struct pointer_map_t *ssa_name_decls;
unsigned i;
unsigned len;
+ bool gen_stack_protect_signal = false;
/* Compute the phase of the stack frame for this function. */
{
@@ -1576,6 +1603,24 @@ expand_used_vars (void)
}
pointer_map_destroy (ssa_name_decls);
+ if (flag_stack_protect == SPCT_FLAG_STRONG)
+ FOR_EACH_LOCAL_DECL (cfun, i, var)
+ 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 = true;
+ break;
+ }
+ }
+
/* At this point all variables on the local_decls with TREE_USED
set are not associated with any block scope. Lay them out. */
@@ -1662,12 +1707,31 @@ 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 ();
+ /* 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 */
+ switch (flag_stack_protect)
+ {
+ case SPCT_FLAG_ALL:
+ create_stack_guard ();
+ break;
+
+ case SPCT_FLAG_STRONG:
+ if (gen_stack_protect_signal ||
+ cfun->calls_alloca || has_protected_decls)
+ create_stack_guard ();
+ break;
+
+ case SPCT_FLAG_DEFAULT:
+ if (cfun->calls_alloca || has_protected_decls)
+ create_stack_guard();
+ break;
+
+ default:
+ ;
+ }
/* Assign rtl to each variable based on these partitions. */
if (stack_vars_num > 0)
diff --git a/gcc/common.opt b/gcc/common.opt
index 6b9b2e1..26b1fbb 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1914,6 +1914,10 @@ fstack-protector-all
Common Report RejectNegative Var(flag_stack_protect, 2)
Use a stack protection method for every function
+fstack-protector-strong
+Common Report RejectNegative Var(flag_stack_protect, 3)
+Use a smart stack protection method for certain functions
+
fstack-usage
Common RejectNegative Var(flag_stack_usage)
Output stack usage information on a per-function basis
diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi
index 4e7b05c..c605b3b 100644
--- a/gcc/doc/cpp.texi
+++ b/gcc/doc/cpp.texi
@@ -2349,6 +2349,10 @@ use.
This macro is defined, with value 2, when
@option{-fstack-protector-all} is
in use.
+@item __SSP_STRONG__
+This macro is defined, with value 3, when
@option{-fstack-protector-strong} is
+in use.
+
@item __SANITIZE_ADDRESS__
This macro is defined, with value 1, when @option{-fsanitize=address} is
in use.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 1652ebc..39971d4 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -406,8 +406,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 -fstrict-aliasing -fstrict-overflow @gol
--fthread-jumps -ftracer -ftree-bit-ccp @gol
+-fstack-protector-all -fstack-protector-strong -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
-ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
@@ -8880,6 +8880,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/gcc.c b/gcc/gcc.c
index bcfbfc8..f098137 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -655,7 +655,7 @@ proper position among the other output files. */
#ifdef TARGET_LIBC_PROVIDES_SSP
#define LINK_SSP_SPEC "%{fstack-protector:}"
#else
-#define LINK_SSP_SPEC
"%{fstack-protector|fstack-protector-all:-lssp_nonshared -lssp}"
+#define LINK_SSP_SPEC
"%{fstack-protector|fstack-protector-strong|fstack-protector-all:-lssp_nonshared
-lssp}"
#endif
#endif
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..c5a52e5
--- /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-*-* rs6000-*-* s390x-*-* } } */
+/* { dg-options "-O2 -fstack-protector-strong" } */
+
+#include<string.h>
+#include<stdlib.h>
+
+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 } } */
--
H.
