| Message ID | 52C6B807.1070203@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 03, 2014 at 02:15:51PM +0100, Florian Weimer wrote: > This patch fixes a loophole in the -fstack-protector-strong > protection. If a function call uses the return slot optimization, > the caller needs stack protector instrumentation because the return > slot is addressable. > > Bootstrapped and regression-tested on x86_64-redhat-linux-gnu, with > C/C++/Java enabled. Okay for trunk? > > -- > Florian Weimer / Red Hat Product Security Team > 2014-01-03 Florian Weimer <fweimer@redhat.com> > > * cfgexpand.c (call_with_return_slot_opt_p): New function. > (expand_used_vars): Emit stack protector instrumentation in strong > mode if call_with_return_slot_opt_p. > > gcc/testsuite/ > > 2014-01-03 Florian Weimer <fweimer@redhat.com> > > * gcc.dg/fstack-protector-strong.c: Add coverage for named return > values. > * g++.dg/fstack-protector-strong.C: Likewise. > > Index: gcc/cfgexpand.c > =================================================================== > --- gcc/cfgexpand.c (revision 206311) > +++ gcc/cfgexpand.c (working copy) > @@ -1599,6 +1599,22 @@ > return 0; > } > > +/* Check if the basic block has a call which uses a return slot. */ > + > +static bool > +call_with_return_slot_opt_p (basic_block bb) > +{ > + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); > + !gsi_end_p (gsi); gsi_next (&gsi)) > + { > + gimple stmt = gsi_stmt (gsi); > + if (gimple_code (stmt) == GIMPLE_CALL That would be is_gimple_call (stmt) instead. Also, I'd think the function is misnamed, given that it checks if there are any calls with return_slot_opt_p in a bb. I think it would be better to move FOR_ALL_BB_FN (bb, cfun) loop also into the function and call it any_call_... Lastly, I wonder if gimple_call_return_slot_opt_p is really what you are after, why does NRV matter here? Isn't what you are looking for instead whether the called function returns value through invisible reference, because then you'll always have some (aggregate) addressable object in the caller's frame and supposedly you are after making sure that the callee doesn't overflow the return object. So, looking at tree-nrv.c, that check would be roughly: if (is_gimple_call (stmt) && gimple_call_lhs (stmt) && aggregate_value_p (TREE_TYPE (gimple_call_lhs (stmt)), gimple_call_fndecl (stmt))) Jakub
On 01/03/2014 07:57 PM, Jakub Jelinek wrote: >> +/* Check if the basic block has a call which uses a return slot. */ >> + >> +static bool >> +call_with_return_slot_opt_p (basic_block bb) >> +{ >> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); >> + !gsi_end_p (gsi); gsi_next (&gsi)) >> + { >> + gimple stmt = gsi_stmt (gsi); >> + if (gimple_code (stmt) == GIMPLE_CALL > > That would be is_gimple_call (stmt) instead. Ah, it's not used consistently everywhere, and I got it from of the leftover places. > Also, I'd think the function is misnamed, given that it checks if there > are any calls with return_slot_opt_p in a bb. I think it would be > better to move FOR_ALL_BB_FN (bb, cfun) loop also into the > function and call it any_call_... I should probably move both loops (the one for declarations and the one for basic blocks) into its own function. > Lastly, I wonder if gimple_call_return_slot_opt_p is really what you are > after, why does NRV matter here? The C code we generate does not construct the returned value in place (presumably because the partial write would be visible with threads, longjmp etc.), unlike the C++ code. That's why I'm interested in instrumenting NRV-able calls only. But gimple_call_return_slot_opt_p doesn't actually give me that. The GIMPLE from the C test case looks like this (before and after applying your proposal): foo11 () { int D.2265; struct B D.2266; D.2266 = global3 (); [return slot optimization] D.2265 = D.2266.a1; return D.2265; } In both cases, SSP instrumentation is applied: .type foo11, @function foo11: .LFB24: .cfi_startproc subq $56, %rsp .cfi_def_cfa_offset 64 movq %rsp, %rdi movq %fs:40, %rax movq %rax, 40(%rsp) xorl %eax, %eax call global3 movq 40(%rsp), %rdx xorq %fs:40, %rdx movl (%rsp), %eax jne .L50 addq $56, %rsp .cfi_remember_state .cfi_def_cfa_offset 8 ret .L50: .cfi_restore_state call __stack_chk_fail .cfi_endproc > Isn't what you are looking for instead > whether the called function returns value through invisible reference, > because then you'll always have some (aggregate) addressable object > in the caller's frame and supposedly you are after making sure that the > callee doesn't overflow the return object. > > So, looking at tree-nrv.c, that check would be roughly: > if (is_gimple_call (stmt) > && gimple_call_lhs (stmt) > && aggregate_value_p (TREE_TYPE (gimple_call_lhs (stmt)), > gimple_call_fndecl (stmt))) When I do that, I get SSP instrumentation even when the struct is small enough to be returned in registers. gimple_call_return_slot_opt_p returns false in this case. So gimple_call_return_slot_opt_p appears to be misnamed (it's an ABI thing, not really an optimization), but it's closer to what I want.
gcc/ 2014-01-03 Florian Weimer <fweimer@redhat.com> * cfgexpand.c (call_with_return_slot_opt_p): New function. (expand_used_vars): Emit stack protector instrumentation in strong mode if call_with_return_slot_opt_p. gcc/testsuite/ 2014-01-03 Florian Weimer <fweimer@redhat.com> * gcc.dg/fstack-protector-strong.c: Add coverage for named return values. * g++.dg/fstack-protector-strong.C: Likewise. Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 206311) +++ gcc/cfgexpand.c (working copy) @@ -1599,6 +1599,22 @@ return 0; } +/* Check if the basic block has a call which uses a return slot. */ + +static bool +call_with_return_slot_opt_p (basic_block bb) +{ + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); + !gsi_end_p (gsi); gsi_next (&gsi)) + { + gimple stmt = gsi_stmt (gsi); + if (gimple_code (stmt) == GIMPLE_CALL + && gimple_call_return_slot_opt_p (stmt)) + return true; + } + return false; +} + /* Expand all variables used in the function. */ static rtx @@ -1669,22 +1685,35 @@ 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)) + { + 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; + } + } + /* The return slot introduces addressable local variables. */ + if (!gen_stack_protect_signal) { - 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)))) + basic_block bb; + FOR_ALL_BB_FN (bb, cfun) { - gen_stack_protect_signal = true; - break; + gen_stack_protect_signal = call_with_return_slot_opt_p (bb); + if (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. */ Index: gcc/testsuite/g++.dg/fstack-protector-strong.C =================================================================== --- gcc/testsuite/g++.dg/fstack-protector-strong.C (revision 206311) +++ gcc/testsuite/g++.dg/fstack-protector-strong.C (working copy) @@ -32,4 +32,39 @@ return global_func (a); } -/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */ +/* Frame addressed exposed through return slot. */ + +struct B +{ + /* Discourage passing this struct in registers. */ + int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10; +}; + +B global_func (); +void noop (); + +int foo3 () +{ + return global_func ().a1; +} + +int foo4 () +{ + try { + noop (); + return 0; + } catch (...) { + return global_func ().a1; + } +} + +int foo5 () +{ + try { + return global_func ().a1; + } catch (...) { + return 0; + } +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 5 } } */ Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c =================================================================== --- gcc/testsuite/gcc.dg/fstack-protector-strong.c (revision 206311) +++ gcc/testsuite/gcc.dg/fstack-protector-strong.c (working copy) @@ -131,4 +131,17 @@ return bb.three; } -/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */ +struct B +{ + /* Discourage passing this struct in registers. */ + int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10; +}; + +struct B global3 (void); + +int foo11 () +{ + return global3 ().a1; +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 11 } } */