This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186).


On 06/30/2017 04:03 PM, Michael Matz wrote:
> Hi,
> 
> On Wed, 28 Jun 2017, Martin Liška wrote:
> 
>> Thanks for the review. I'm not so familiar with RTL, but hopefully new 
>> version of the patch should do it properly. Idea is to come up with a 
>> new asan_stack_birth_insn that points after place where stack is ready 
>> to use (when one uses use-after-return). And then in function.c 
>> static_chain_decl and nonlocal_goto_save_area expansion is generated to 
>> a new sequence and the sequence is put after the asan_stack_birth_insn.
> 
> That has the same problem.
> 
> The code for function start is conceptually like this:
> 
> entry_point:
>   setup return space:
>     on some targets the address of the return buffer is passed in a 
>     certain incoming arg (i.e. it's like an argument)
>   setup arguments:
>     store away incoming registers into pseudo
>     load stack slot arguments (or put them there, all target dependend)
>   * point 1 where you insert code *
>   setup static chain:
>     conceptually this is just a hidden additional function argument
>   setup nonlocal goto save area
> 
> If you simply create any other code inside this sequence you potentially 
> have the problem of clobbering any of the incoming hardregs.  In simple
> examples you might not notice when the register allocator heeds the 
> hardreg uses, but if you for instance call other functions in there you 
> most likely clobber some.  E.g. r10 (the incoming static chain pointer on 
> x86-64) isn't preserved across function calls.  So if you call a function 
> at point 1 above you clobber r10, but the code for setting up the static 
> chain needs it as input.

Thanks Michael.

Now I got it as I understand the problematic of usage of r10 register. Actually
it's easy to come up with a test-case that breaks that:

$ cat /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c
/* PR sanitizer/81186 */
/* { dg-do run } */

int
main ()
{
  __label__ l;
  void f ()
    {
      int a[123];

	goto l;
    }

  f ();
l:
  return 0;
}

where:

f.2139:
.LASANPC1:
.LFB1:
	.cfi_startproc
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset 6, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register 6
	pushq	%rbx
	subq	$600, %rsp
	.cfi_offset 3, -24
	leaq	-592(%rbp), %rbx
	cmpl	$0, __asan_option_detect_stack_use_after_return(%rip)
	je	.L1
	movl	$576, %edi
	call	__asan_stack_malloc_4 <--- here clobbering of r10 happens
	testq	%rax, %rax
	je	.L1
	movq	%rax, %rbx
.L1:
	movq	%r10, %rdx <- use after it's clobbered
	movq	%r10, -600(%rbp)
	movq	$1102416563, (%rbx)


> 
> So you need to find some other solution of setting up the stack for ASAN.  
> And it'd be best if that solution doesn't require inserting code inside 
> the above sequence of parameter setup instructions, and you certainly 
> can't call any functions inside that sequence.  It might mean that you 
> can't track the static chain place or the nonlocal goto save area.  You 
> also don't track the parameter stack slots, right?

IIUC parameter stack slots are not sanitized.

I will think about it more ;)
Martin

> 
> 
> Ciao,
> Michael.
>>
>>>
>>> Also if you put something in front of the static_chain_decl initialization 
>>> (as you do if you move the parm_birth_insn in front of it) you'd have to 
>>> make sure that the incoming hidding parameter containing the static chain 
>>> (r10 on x86_64) isn't clobbered from function start up to that point.  
>>> So that won't work either generally.
>>>
>>> I don't know what exactly is the issue with calling 
>>> __asan_handle_no_return before the other instructions emitted by 
>>> expand_used_vars.  Either it shouldn't be called for the static chain 
>>> (i.e. not instrumented) or whatever setup asan needs needs to happen in 
>>> front of the static chain setup, but then without clobbering the incoming 
>>> static chain param (!).
>>
>> Here I need to have FRAME variable to be sanitized as seen in pr78541.c
>> and pr78541-2.c test-cases.
>>
>> Thoughts?
>> Thanks,
>> Martin
>>
>>>
>>>
>>> Ciao,
>>> Michael.
>>>>
>>>> expanded cfun->static_chain_decl:
>>>>
>>>> (note 1 0 5 NOTE_INSN_DELETED)
>>>> (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
>>>> (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ])
>>>>         (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1
>>>>      (nil))
>>>> (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
>>>>                 (const_int -8 [0xfffffffffffffff8])) [0  S8 A64])
>>>>         (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1
>>>>      (nil))
>>>> (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG)
>>>> (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ])
>>>>         (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1
>>>>      (nil))
>>>> (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41]  <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8])
>>>>         (const_int 0 [0])) "pr81186.c":5 -1
>>>>      (expr_list:REG_EH_REGION (const_int 0 [0])
>>>>         (nil))
>>>>     (nil))
>>>>
>>>> expanded cfun->nonlocal_goto_save_area:
>>>>
>>>> (note 1 0 34 NOTE_INSN_DELETED)
>>>> (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
>>>> (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95)
>>>>                 (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64])
>>>>         (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1
>>>>      (nil))
>>>> (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95)
>>>>                 (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64])
>>>>         (reg/f:DI 7 sp)) "pr81186.c":3 -1
>>>>      (nil))
>>>> (insn 2 32 3 2 (parallel [
>>>>             (set (reg:DI 96)
>>>>                 (plus:DI (reg/f:DI 82 virtual-stack-vars)
>>>>                     (const_int -96 [0xffffffffffffffa0])))
>>>>             (clobber (reg:CC 17 flags))
>>>>         ]) "pr81186.c":3 -1
>>>>      (nil))
>>>> (insn 3 2 4 2 (set (reg:DI 97)
>>>>         (reg:DI 96)) "pr81186.c":3 -1
>>>>      (nil))
>>>> (insn 4 3 5 2 (set (reg:CCZ 17 flags)
>>>>         (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40]  <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32])
>>>>             (const_int 0 [0]))) "pr81186.c":3 -1
>>>>      (nil))
>>>>
>>>> And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG.
>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>> Martin
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2017-06-27  Martin Liska  <mliska@suse.cz>
>>>>
>>>>         PR sanitize/81186
>>>> 	* function.c (expand_function_start): Move static chain and non-local
>>>> 	goto init after NOTE_INSN_FUNCTION_BEG.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2017-06-27  Martin Liska  <mliska@suse.cz>
>>>>
>>>>         PR sanitize/81186
>>>> 	* gcc.dg/asan/pr81186.c: New test.
>>>> ---
>>>>  gcc/function.c                      | 18 +++++++++---------
>>>>  gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++
>>>>  2 files changed, 22 insertions(+), 9 deletions(-)
>>>>  create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c
>>>>
>>>>
>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]