diff mbox

Add a new option "-fstack-protector-strong"

Message ID 516DF1A9.8050300@google.com
State New
Headers show

Commit Message

Han Shen April 17, 2013, 12:49 a.m. UTC
Hi Florian, thanks for the review!

On Tue, Apr 16, 2013 at 6:32 AM, Florian Weimer <fweimer@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  <shenhan@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  <shenhan@google.com>
	* c-cppbuiltin.c (c_cpp_builtins): Added "__SSP_STRONG__=3".

gcc/testsuite/ChangeLog
2013-04-16  Han Shen  <shenhan@google.com>
	Test cases for '-fstack-protector-strong'.
	* gcc.dg/fstack-protector-strong.c: New.
	* g++.dg/fstack-protector-strong.C: New.




--
H.

Comments

Florian Weimer April 17, 2013, 9:26 a.m. UTC | #1
On 04/17/2013 02:49 AM, Han Shen wrote:
>>> +  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?

I see.  Let's use the constants for now.

>> 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)

Thunderbird mangles patches as well, but I was able to repair the 
damage.  When using Thunderbird, please send the patch as a text file 
attachment.  You can put the changelog snippets at the beginning of the 
file as well.  This way, everything is sent out unchanged.

>> 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'.

Thanks.  I think the comment is now redundant because it matches the 
code almost word-for-word. 8-)

> 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.

C++ const references can bind to rvalues.

But I'm more worried about the interaction with the return value 
optimization.  Consider this C++ code:

struct S {
   S();
   int a;
   int b;
   int c;
   int d;
   int e;
};

void f1(int *);

S f2()
{
   S s;
   f1(&s.a);
   return s;
}

S g2();

void g3()
{
   S s = g2();
}

void g3b(const S&);

void g3b()
{
   g3b(g2());
}

With your patch and -O2 -fstack-protector-strong, this generates the 
following assembly:

	.globl	_Z2f2v
	.type	_Z2f2v, @function
_Z2f2v:
.LFB0:
	.cfi_startproc
	pushq	%rbx
	.cfi_def_cfa_offset 16
	.cfi_offset 3, -16
	movq	%rdi, %rbx
	call	_ZN1SC1Ev
	movq	%rbx, %rdi
	call	_Z2f1Pi
	movq	%rbx, %rax
	popq	%rbx
	.cfi_def_cfa_offset 8
	ret
	.cfi_endproc
.LFE0:
	.size	_Z2f2v, .-_Z2f2v
	.p2align 4,,15
	.globl	_Z2g3v
	.type	_Z2g3v, @function
_Z2g3v:
.LFB1:
	.cfi_startproc
	subq	$40, %rsp
	.cfi_def_cfa_offset 48
	movq	%rsp, %rdi
	call	_Z2g2v
	addq	$40, %rsp
	.cfi_def_cfa_offset 8
	ret
	.cfi_endproc
.LFE1:
	.size	_Z2g3v, .-_Z2g3v
	.p2align 4,,15
	.globl	_Z3g3bv
	.type	_Z3g3bv, @function
_Z3g3bv:
.LFB2:
	.cfi_startproc
	subq	$40, %rsp
	.cfi_def_cfa_offset 48
	movq	%rsp, %rdi
	movq	%fs:40, %rax
	movq	%rax, 24(%rsp)
	xorl	%eax, %eax
	call	_Z2g2v
	movq	%rsp, %rdi
	call	_Z3g3bRK1S
	movq	24(%rsp), %rax
	xorq	%fs:40, %rax
	jne	.L9
	addq	$40, %rsp
	.cfi_remember_state
	.cfi_def_cfa_offset 8
	ret
.L9:
	.cfi_restore_state
	.p2align 4,,6
	call	__stack_chk_fail
	.cfi_endproc
.LFE2:
	.size	_Z3g3bv, .-_Z3g3bv

Here, g3b() is correctly instrumented, and f2() does not need 
instrumentation (because space for the returned object is not part of 
the local frame).  But an address on the stack escapes in g3() and is 
used for the return value of the call to g2().  This requires 
instrumentation, which is missing in this example.

I suppose this can be handled in a follow-up patch if necessary.

> ChangeLog and patch below --
>
> gcc/ChangeLog
> 2013-04-16  Han Shen  <shenhan@google.com>
>      * cfgexpand.c (record_or_union_type_has_array_p): Helper function
>      to check if a record or union contains an array field.

I think the GNU convention is to write only this:

	* cfgexpand.c (record_or_union_type_has_array_p): New function.

>      (expand_used_vars): Add logic handling '-fstack-protector-strong'.
>      * common.opt (fstack-protector-all): New option.

Should be "fstack-protector-strong".
Han Shen April 17, 2013, 6:40 p.m. UTC | #2
Thanks.

On Wed, Apr 17, 2013 at 2:26 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/17/2013 02:49 AM, Han Shen wrote:
>>> 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)
>
>
> Thunderbird mangles patches as well, but I was able to repair the damage.
> When using Thunderbird, please send the patch as a text file attachment.
> You can put the changelog snippets at the beginning of the file as well.
> This way, everything is sent out unchanged.
>
Patch sent as attachment.
>
>
>>> 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'.
>
>
> Thanks.  I think the comment is now redundant because it matches the code
> almost word-for-word. 8-)
>
Comment deleted.
>
>> 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.
>
>
> C++ const references can bind to rvalues.
> 2013-04-11  Jakub Jelinek  <jakub@redhat.com>
> But I'm more worried about the interaction with the return value
> optimization.  Consider this C++ code:
>
> struct S {
>   S();
>   int a;
>   int b;
>   int c;
>   int d;
>   int e;
> };
>
> void f1(int *);
>
> S f2()
> {
>   S s;
>   f1(&s.a);
>   return s;
> }
>
> S g2();
>
> void g3()
> {
>   S s = g2();
> }
>
> void g3b(const S&);
>
> void g3b()
> {
>   g3b(g2());
> }
>
> With your patch and -O2 -fstack-protector-strong, this generates the
> following assembly:
>
>         .globl  _Z2f2v
>         .type   _Z2f2v, @function
> _Z2f2v:
> .LFB0:
>         .cfi_startproc
>         pushq   %rbx
>         .cfi_def_cfa_offset 16
>         .cfi_offset 3, -16
>         movq    %rdi, %rbx
>         call    _ZN1SC1Ev
>         movq    %rbx, %rdi
>         call    _Z2f1Pi
>         movq    %rbx, %rax
>         popq    %rbx
>         .cfi_def_cfa_offset 8
>         ret
>         .cfi_endproc
> .LFE0:
>         .size   _Z2f2v, .-_Z2f2v
>         .p2align 4,,15
>         .globl  _Z2g3v
>         .type   _Z2g3v, @function
> _Z2g3v:
> .LFB1:
>         .cfi_startproc
>         subq    $40, %rsp
>         .cfi_def_cfa_offset 48
>         movq    %rsp, %rdi
>         call    _Z2g2v
>         addq    $40, %rsp
>         .cfi_def_cfa_offset 8
>         ret
>         .cfi_endproc
> .LFE1:
>         .size   _Z2g3v, .-_Z2g3v
>         .p2align 4,,15
>         .globl  _Z3g3bv
>         .type   _Z3g3bv, @function
> _Z3g3bv:
> .LFB2:
>         .cfi_startproc
>         subq    $40, %rsp
>         .cfi_def_cfa_offset 48
>         movq    %rsp, %rdi
>         movq    %fs:40, %rax
>         movq    %rax, 24(%rsp)
>         xorl    %eax, %eax
>         call    _Z2g2v
>         movq    %rsp, %rdi
>         call    _Z3g3bRK1S
>         movq    24(%rsp), %rax
>         xorq    %fs:40, %rax
>         jne     .L9
>         addq    $40, %rsp
>         .cfi_remember_state
>         .cfi_def_cfa_offset 8
>         ret
> .L9:
>         .cfi_restore_state
>         .p2align 4,,6
>         call    __stack_chk_fail
>         .cfi_endproc
> .LFE2:
>         .size   _Z3g3bv, .-_Z3g3bv
>
> Here, g3b() is correctly instrumented, and f2() does not need
> instrumentation (because space for the returned object is not part of the
> local frame).  But an address on the stack escapes in g3() and is used for
> the return value of the call to g2().  This requires instrumentation, which
> is missing in this example.
>
> I suppose this can be handled in a follow-up patch if necessary.
>
Thanks for the case. Yeah, I see the problem here - in cfgexpand phase
- where functions being scanned for stack protection - the tree
representation has no indication that a local address is computed,
just as listed below -
  void g3() ()
  {
    struct S s;
    <bb 2>:
    s = g2 (); [return slot optimization]
    s ={v} {CLOBBER};
    return;
  }
One solution might be to scan for the "[return slot optimization]" tag
in the tree. I'll post later another patch to address this.
>
>> ChangeLog and patch below --
>>
>> gcc/ChangeLog
>> 2013-04-16  Han Shen  <shenhan@google.com>
>>      * cfgexpand.c (record_or_union_type_has_array_p): Helper function
>>      to check if a record or union contains an array field.
>
>
> I think the GNU convention is to write only this:
>
>         * cfgexpand.c (record_or_union_type_has_array_p): New function.
Done.
>
>
>>      (expand_used_vars): Add logic handling '-fstack-protector-strong'.
>>      * common.opt (fstack-protector-all): New option.
>
>
> Should be "fstack-protector-strong".
Done.
>
>
> --
> Florian Weimer / Red Hat Product Security Team

Patch attached as plain txt.

Thanks,
H.
Han Shen May 7, 2013, 5:22 p.m. UTC | #3
Updated patch according to Jeff Law's comments (
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00038.html )

Thanks,
H.

On Wed, Apr 17, 2013 at 11:40 AM, Han Shen(沈涵) <shenhan@google.com> wrote:
> Thanks.
>
> On Wed, Apr 17, 2013 at 2:26 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 04/17/2013 02:49 AM, Han Shen wrote:
>>>> 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)
>>
>>
>> Thunderbird mangles patches as well, but I was able to repair the damage.
>> When using Thunderbird, please send the patch as a text file attachment.
>> You can put the changelog snippets at the beginning of the file as well.
>> This way, everything is sent out unchanged.
>>
> Patch sent as attachment.
>>
>>
>>>> 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'.
>>
>>
>> Thanks.  I think the comment is now redundant because it matches the code
>> almost word-for-word. 8-)
>>
> Comment deleted.
>>
>>> 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.
>>
>>
>> C++ const references can bind to rvalues.
>> 2013-04-11  Jakub Jelinek  <jakub@redhat.com>
>> But I'm more worried about the interaction with the return value
>> optimization.  Consider this C++ code:
>>
>> struct S {
>>   S();
>>   int a;
>>   int b;
>>   int c;
>>   int d;
>>   int e;
>> };
>>
>> void f1(int *);
>>
>> S f2()
>> {
>>   S s;
>>   f1(&s.a);
>>   return s;
>> }
>>
>> S g2();
>>
>> void g3()
>> {
>>   S s = g2();
>> }
>>
>> void g3b(const S&);
>>
>> void g3b()
>> {
>>   g3b(g2());
>> }
>>
>> With your patch and -O2 -fstack-protector-strong, this generates the
>> following assembly:
>>
>>         .globl  _Z2f2v
>>         .type   _Z2f2v, @function
>> _Z2f2v:
>> .LFB0:
>>         .cfi_startproc
>>         pushq   %rbx
>>         .cfi_def_cfa_offset 16
>>         .cfi_offset 3, -16
>>         movq    %rdi, %rbx
>>         call    _ZN1SC1Ev
>>         movq    %rbx, %rdi
>>         call    _Z2f1Pi
>>         movq    %rbx, %rax
>>         popq    %rbx
>>         .cfi_def_cfa_offset 8
>>         ret
>>         .cfi_endproc
>> .LFE0:
>>         .size   _Z2f2v, .-_Z2f2v
>>         .p2align 4,,15
>>         .globl  _Z2g3v
>>         .type   _Z2g3v, @function
>> _Z2g3v:
>> .LFB1:
>>         .cfi_startproc
>>         subq    $40, %rsp
>>         .cfi_def_cfa_offset 48
>>         movq    %rsp, %rdi
>>         call    _Z2g2v
>>         addq    $40, %rsp
>>         .cfi_def_cfa_offset 8
>>         ret
>>         .cfi_endproc
>> .LFE1:
>>         .size   _Z2g3v, .-_Z2g3v
>>         .p2align 4,,15
>>         .globl  _Z3g3bv
>>         .type   _Z3g3bv, @function
>> _Z3g3bv:
>> .LFB2:
>>         .cfi_startproc
>>         subq    $40, %rsp
>>         .cfi_def_cfa_offset 48
>>         movq    %rsp, %rdi
>>         movq    %fs:40, %rax
>>         movq    %rax, 24(%rsp)
>>         xorl    %eax, %eax
>>         call    _Z2g2v
>>         movq    %rsp, %rdi
>>         call    _Z3g3bRK1S
>>         movq    24(%rsp), %rax
>>         xorq    %fs:40, %rax
>>         jne     .L9
>>         addq    $40, %rsp
>>         .cfi_remember_state
>>         .cfi_def_cfa_offset 8
>>         ret
>> .L9:
>>         .cfi_restore_state
>>         .p2align 4,,6
>>         call    __stack_chk_fail
>>         .cfi_endproc
>> .LFE2:
>>         .size   _Z3g3bv, .-_Z3g3bv
>>
>> Here, g3b() is correctly instrumented, and f2() does not need
>> instrumentation (because space for the returned object is not part of the
>> local frame).  But an address on the stack escapes in g3() and is used for
>> the return value of the call to g2().  This requires instrumentation, which
>> is missing in this example.
>>
>> I suppose this can be handled in a follow-up patch if necessary.
>>
> Thanks for the case. Yeah, I see the problem here - in cfgexpand phase
> - where functions being scanned for stack protection - the tree
> representation has no indication that a local address is computed,
> just as listed below -
>   void g3() ()
>   {
>     struct S s;
>     <bb 2>:
>     s = g2 (); [return slot optimization]
>     s ={v} {CLOBBER};
>     return;
>   }
> One solution might be to scan for the "[return slot optimization]" tag
> in the tree. I'll post later another patch to address this.
>>
>>> ChangeLog and patch below --
>>>
>>> gcc/ChangeLog
>>> 2013-04-16  Han Shen  <shenhan@google.com>
>>>      * cfgexpand.c (record_or_union_type_has_array_p): Helper function
>>>      to check if a record or union contains an array field.
>>
>>
>> I think the GNU convention is to write only this:
>>
>>         * cfgexpand.c (record_or_union_type_has_array_p): New function.
> Done.
>>
>>
>>>      (expand_used_vars): Add logic handling '-fstack-protector-strong'.
>>>      * common.opt (fstack-protector-all): New option.
>>
>>
>> Should be "fstack-protector-strong".
> Done.
>>
>>
>> --
>> Florian Weimer / Red Hat Product Security Team
>
> Patch attached as plain txt.
>
> Thanks,
> H.
diff mbox

Patch

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 } } */