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

[Martin Liška]

On 07/25/2017 02:49 PM, Jakub Jelinek wrote:
> On Tue, Jul 18, 2017 at 10:38:50AM +0200, Martin Liška wrote:
>> 2017-06-27  Martin Liska  
>>
>> PR sanitize/81186
> 
> 8 spaces instead of tab?
> 
>>  * function.c (expand_function_start): Set parm_birth_insn after
>>  static chain is initialized.
> 
> I don't like this description, after all, parm_birth_insn was set
> after static chain initialization before too (just not right after it
> in some cases).  The important change is that you've moved parm_birth_insn
> before the nonlocal_goto_save_area setup code, so IMHO the ChangeLog entry
> should say that.

Both notes fixed and the patch has been also installed to GCC 7 branch.
Thanks,
Martin

> 
> As for the patch itself, there are many spots which insert some code
> before or after parm_birth_insn or spots tied to the NOTE_INSN_FUNCTION_BEG
> note, but I'd hope nothing inserted there can actually call functions that
> perform non-local gotos, so I think the patch is fine.  And for debug info
> experience which is also related to NOTE_INSN_FUNCTION_BEG, I think the nl
> goto save area is nothing that can be seen in the debugger unless you know
> where it is, so the only change might be if you put a breakpoint on the end
> of prologue (i.e. NOTE_INSN_FUNCTION_BEG) and call from inferios some
> function that performs a non-local goto.  I think there are no barriers
> on that initialization anyway, so scheduler can move it around.
> 
> Thus, ok for trunk/7.2 with the above suggested ChangeLog change.
> 
>   Jakub
> 

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

[Jakub Jelinek]

On Tue, Jul 18, 2017 at 10:38:50AM +0200, Martin Liška wrote:
> 2017-06-27  Martin Liska  
> 
> PR sanitize/81186

8 spaces instead of tab?

>   * function.c (expand_function_start): Set parm_birth_insn after
>   static chain is initialized.

I don't like this description, after all, parm_birth_insn was set
after static chain initialization before too (just not right after it
in some cases).  The important change is that you've moved parm_birth_insn
before the nonlocal_goto_save_area setup code, so IMHO the ChangeLog entry
should say that.

As for the patch itself, there are many spots which insert some code
before or after parm_birth_insn or spots tied to the NOTE_INSN_FUNCTION_BEG
note, but I'd hope nothing inserted there can actually call functions that
perform non-local gotos, so I think the patch is fine.  And for debug info
experience which is also related to NOTE_INSN_FUNCTION_BEG, I think the nl
goto save area is nothing that can be seen in the debugger unless you know
where it is, so the only change might be if you put a breakpoint on the end
of prologue (i.e. NOTE_INSN_FUNCTION_BEG) and call from inferios some
function that performs a non-local goto.  I think there are no barriers
on that initialization anyway, so scheduler can move it around.

Thus, ok for trunk/7.2 with the above suggested ChangeLog change.

Jakub

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

[Martin Liška]

PING^1

Jakub can you please take a look? I would like to have it in 7.2 if possible.

Thanks,
Martin

On 07/18/2017 10:38 AM, Martin Liška wrote:
> On 07/17/2017 03:15 PM, Michael Matz wrote:
>> Hello,
>>
>> On Mon, 17 Jul 2017, Martin Liška wrote:
>>
>>> which does all the stack preparation (including the problematic call to 
>>> __asan_stack_malloc_N).
>>>
>>> Note that this code still should be placed before parm_birth_note as we 
>>> cant's say that params are ready before a fake stack is prepared.
>>
>> Yes, understood.
>>
>>> Then we generate code that loads the implicit chain argument:
>>>
>>> (gdb) p debug_rtx_list(get_insns(), 100)
>>> (note 1 0 37 NOTE_INSN_DELETED)
>>>
>>> (note 37 1 38 NOTE_INSN_FUNCTION_BEG)
>>>
>>> (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ])
>>> (reg:DI 39 r10 [ CHAIN.1 ])) 
>>> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>>>  (nil))
>>>
>>> (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
>>> (const_int -584 [0xfdb8])) [0  S8 A64])
>>> (reg:DI 39 r10 [ CHAIN.1 ])) 
>>> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>>>  (nil))
>>>
>>> Which is problematic as using virtual-stack-vars which should point to 
>>> fake stack done by AddressSanitizer in __asan_stack_malloc_N.
>>
>> If anything, then only the stack access is problematic, i.e. the last 
>> instruction.  I don't understand why that should be problematic, though.  
> 
> Hi.
> 
> Thanks one more time, it's really educative this PR and whole problematic of 
> function prologue.
> So short answer for your email: marking parm_birth_insn after static chain 
> init solves the problem :)
> It's because:
> 
> (insn 2 1 3 (set (reg/f:DI 100 [ CHAIN.2 ])
> (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1
>  (nil))
> 
> (insn 3 2 4 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
> (const_int -8 [0xfff8])) [0  S8 A64])
> (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1
>  (nil))
> 
> is just storage of  from caller where content of the FRAME struct 
> lives on stack (and thus on
> shadow stack). That said it's perfectly fine to store  to real stack of 
> callee.
> 
> Thus I'm going to test attached patch.
> 
> P.S. One interesting side effect of how static chain is implemented:
> 
> Consider:
> 
> int
> main ()
> {
>   __label__ l;
>   int buffer[100];
>   void f ()
>   {
> int a[123];
> *([0] - 4) = 123;
> 
> goto l;
>   }
> 
>   f ();
> l:
>   return 0;
> }
> 
> It's funny that *([0] - 4) actually corrupts __nl_goto_buf and we end 
> up with a
> dead signal:
> 
> ASAN:DEADLYSIGNAL
> =
> ==30888==ERROR: AddressSanitizer: SEGV on unknown address 0x (pc 
> 0x bp 0x sp 0x7ffe049b T0)
> 
> Thanks,
> Martin
> 
>> Probably because I don't know much about the ASAN implementation.  But why 
>> should there be something magic about using the non-asan stack?  Most 
>> local variable accesses are rewritten to be in terms of the fake stack, 
>> but those that aren't could use the normal stack just fine, can't they?
>>
>> If that really is a problem then that could also be rectified by splitting 
>> the static_chain_decl in expand_function_start a bit, ala this:
>>
>>   if (cfun->static_chain_decl) {
>> all code except the last "if (!optimize) store-into-stack"
>>   }
>>   emit_note; parm_birth_insn = ...
>>   if (cfun->static_chain_decl && !optimize) {
>> store into assign_stack_local
>>   }
>>
>> (requires moving some local variable to an outer scope, but hey).
>>
>> But what you say above mystifies me.  You claim that access via 
>> virtual-stack-vars is problematic before the shadow stack is created by 
>> ASAN.  But the whole parameter setup always uses such local stack storage 
>> whenever it needs.  And those definitely happen before the ASAN setup.  
>> See the subroutines of assign_parms, (e.g. assign_parm_setup_block and 
>> assign_parm_setup_stack).  You might need to use special function argument 
>> types or special ABIs to trigger this, though you should be able to find 
>> some cases to trigger also on i386 or x86_64.
>>
>> So, if the stack access for the static chain is problematic I don't see 
>> why the stack accesses for the parameters are not.  And if they indeed are 
>> problematic, then something is confused within ASAN, and the fix for that 
>> confusion is not to move parm_birth_insn, but something else (I can't say 
>> what, as I don't know much about how ASAN is supposed to work in such 
>> situations).
>>
>>
>> Ciao,
>> Michael.
>>
> 

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

[Martin Liška]

On 07/17/2017 03:15 PM, Michael Matz wrote:
> Hello,
> 
> On Mon, 17 Jul 2017, Martin Liška wrote:
> 
>> which does all the stack preparation (including the problematic call to 
>> __asan_stack_malloc_N).
>>
>> Note that this code still should be placed before parm_birth_note as we 
>> cant's say that params are ready before a fake stack is prepared.
> 
> Yes, understood.
> 
>> Then we generate code that loads the implicit chain argument:
>>
>> (gdb) p debug_rtx_list(get_insns(), 100)
>> (note 1 0 37 NOTE_INSN_DELETED)
>>
>> (note 37 1 38 NOTE_INSN_FUNCTION_BEG)
>>
>> (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ])
>> (reg:DI 39 r10 [ CHAIN.1 ])) 
>> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>>  (nil))
>>
>> (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
>> (const_int -584 [0xfdb8])) [0  S8 A64])
>> (reg:DI 39 r10 [ CHAIN.1 ])) 
>> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>>  (nil))
>>
>> Which is problematic as using virtual-stack-vars which should point to 
>> fake stack done by AddressSanitizer in __asan_stack_malloc_N.
> 
> If anything, then only the stack access is problematic, i.e. the last 
> instruction.  I don't understand why that should be problematic, though.  

Hi.

Thanks one more time, it's really educative this PR and whole problematic of 
function prologue.
So short answer for your email: marking parm_birth_insn after static chain init 
solves the problem :)
It's because:

(insn 2 1 3 (set (reg/f:DI 100 [ CHAIN.2 ])
(reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1
 (nil))

(insn 3 2 4 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
(const_int -8 [0xfff8])) [0  S8 A64])
(reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1
 (nil))

is just storage of  from caller where content of the FRAME struct lives 
on stack (and thus on
shadow stack). That said it's perfectly fine to store  to real stack of 
callee.

Thus I'm going to test attached patch.

P.S. One interesting side effect of how static chain is implemented:

Consider:

int
main ()
{
  __label__ l;
  int buffer[100];
  void f ()
  {
int a[123];
*([0] - 4) = 123;

goto l;
  }

  f ();
l:
  return 0;
}

It's funny that *([0] - 4) actually corrupts __nl_goto_buf and we end up 
with a
dead signal:

ASAN:DEADLYSIGNAL
=
==30888==ERROR: AddressSanitizer: SEGV on unknown address 0x (pc 
0x bp 0x sp 0x7ffe049b T0)

Thanks,
Martin

> Probably because I don't know much about the ASAN implementation.  But why 
> should there be something magic about using the non-asan stack?  Most 
> local variable accesses are rewritten to be in terms of the fake stack, 
> but those that aren't could use the normal stack just fine, can't they?
> 
> If that really is a problem then that could also be rectified by splitting 
> the static_chain_decl in expand_function_start a bit, ala this:
> 
>   if (cfun->static_chain_decl) {
> all code except the last "if (!optimize) store-into-stack"
>   }
>   emit_note; parm_birth_insn = ...
>   if (cfun->static_chain_decl && !optimize) {
> store into assign_stack_local
>   }
> 
> (requires moving some local variable to an outer scope, but hey).
> 
> But what you say above mystifies me.  You claim that access via 
> virtual-stack-vars is problematic before the shadow stack is created by 
> ASAN.  But the whole parameter setup always uses such local stack storage 
> whenever it needs.  And those definitely happen before the ASAN setup.  
> See the subroutines of assign_parms, (e.g. assign_parm_setup_block and 
> assign_parm_setup_stack).  You might need to use special function argument 
> types or special ABIs to trigger this, though you should be able to find 
> some cases to trigger also on i386 or x86_64.
> 
> So, if the stack access for the static chain is problematic I don't see 
> why the stack accesses for the parameters are not.  And if they indeed are 
> problematic, then something is confused within ASAN, and the fix for that 
> confusion is not to move parm_birth_insn, but something else (I can't say 
> what, as I don't know much about how ASAN is supposed to work in such 
> situations).
> 
> 
> Ciao,
> Michael.
> 

>From 13d08eb4c7d1ff7cddd130acad405ec343cb826f Mon Sep 17 00:00:00 2001
From: marxin 
Date: Thu, 13 Jul 2017 13:37:47 +0200
Subject: [PATCH] Move static chain and non-local goto init after
 NOTE_INSN_FUNCTION_BEG

gcc/ChangeLog:

2017-06-27  Martin Liska  

PR sanitize/81186
	* function.c (expand_function_start): Set parm_birth_insn after
	static chain is initialized.

gcc/testsuite/ChangeLog:

2017-06-27  Martin Liska  

PR sanitize/81186
	* gcc.dg/asan/pr81186.c: New test.
---
 gcc/function.c  | 20 

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

[Michael Matz]

Hello,

On Mon, 17 Jul 2017, Martin Liška wrote:

> which does all the stack preparation (including the problematic call to 
> __asan_stack_malloc_N).
> 
> Note that this code still should be placed before parm_birth_note as we 
> cant's say that params are ready before a fake stack is prepared.

Yes, understood.

> Then we generate code that loads the implicit chain argument:
> 
> (gdb) p debug_rtx_list(get_insns(), 100)
> (note 1 0 37 NOTE_INSN_DELETED)
> 
> (note 37 1 38 NOTE_INSN_FUNCTION_BEG)
> 
> (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ])
> (reg:DI 39 r10 [ CHAIN.1 ])) 
> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>  (nil))
> 
> (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
> (const_int -584 [0xfdb8])) [0  S8 A64])
> (reg:DI 39 r10 [ CHAIN.1 ])) 
> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>  (nil))
> 
> Which is problematic as using virtual-stack-vars which should point to 
> fake stack done by AddressSanitizer in __asan_stack_malloc_N.

If anything, then only the stack access is problematic, i.e. the last 
instruction.  I don't understand why that should be problematic, though.  
Probably because I don't know much about the ASAN implementation.  But why 
should there be something magic about using the non-asan stack?  Most 
local variable accesses are rewritten to be in terms of the fake stack, 
but those that aren't could use the normal stack just fine, can't they?

If that really is a problem then that could also be rectified by splitting 
the static_chain_decl in expand_function_start a bit, ala this:

  if (cfun->static_chain_decl) {
all code except the last "if (!optimize) store-into-stack"
  }
  emit_note; parm_birth_insn = ...
  if (cfun->static_chain_decl && !optimize) {
store into assign_stack_local
  }

(requires moving some local variable to an outer scope, but hey).

But what you say above mystifies me.  You claim that access via 
virtual-stack-vars is problematic before the shadow stack is created by 
ASAN.  But the whole parameter setup always uses such local stack storage 
whenever it needs.  And those definitely happen before the ASAN setup.  
See the subroutines of assign_parms, (e.g. assign_parm_setup_block and 
assign_parm_setup_stack).  You might need to use special function argument 
types or special ABIs to trigger this, though you should be able to find 
some cases to trigger also on i386 or x86_64.

So, if the stack access for the static chain is problematic I don't see 
why the stack accesses for the parameters are not.  And if they indeed are 
problematic, then something is confused within ASAN, and the fix for that 
confusion is not to move parm_birth_insn, but something else (I can't say 
what, as I don't know much about how ASAN is supposed to work in such 
situations).


Ciao,
Michael.

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

[Martin Liška]

On 07/14/2017 03:42 PM, Michael Matz wrote:
> Hi,
> 
> On Thu, 13 Jul 2017, Martin Liška wrote:
> 
>> Hopefully following patch will fix that. I returned to the first version 
>> and saved/restored static_chain register before/after 
>> __asan_stack_malloc.
> 
> It should also work if you emit the parm_birth_note after the static chain 
> is set up (not before it), but before you store into the 
> nonlocal_goto_save_area.  With that you don't need to worry about 
> clobbering the incoming static chain with the asan setup.

Unfortunately it does not work. First asan_emit_stack_protection is executed, 
which creates:

#0  0x009850f4 in expand_used_vars () at ../../gcc/cfgexpand.c:2233
#1  0x00992ab7 in (anonymous namespace)::pass_expand::execute 
(this=0x28c02f0, fun=0x2c1b60b0) at ../../gcc/cfgexpand.c:6232
#2  0x00e0d3a8 in execute_one_pass (pass=0x28c02f0) at 
../../gcc/passes.c:2492

which does all the stack preparation (including the problematic call to 
__asan_stack_malloc_N).

Note that this code still should be placed before parm_birth_note as we cant's 
say that params are
ready before a fake stack is prepared.

Then we generate code that loads the implicit chain argument:

(gdb) p debug_rtx_list(get_insns(), 100)
(note 1 0 37 NOTE_INSN_DELETED)

(note 37 1 38 NOTE_INSN_FUNCTION_BEG)

(insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ])
(reg:DI 39 r10 [ CHAIN.1 ])) 
"/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
 (nil))

(insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
(const_int -584 [0xfdb8])) [0  S8 A64])
(reg:DI 39 r10 [ CHAIN.1 ])) 
"/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
 (nil))

Which is problematic as using virtual-stack-vars which should point to fake 
stack done by AddressSanitizer
in __asan_stack_malloc_N.

That said both parts (ASAN fake stack init and CHAIN load from implicit 
argument) are before parm birth actions
that should be aware each other. Thus my previous patch preserves the r10 
register on x86_64.

Thanks,
Martin

> 
> Can you test that?  It would better reflect the intent of this note (the 
> static chain being an implicit parameter, but the nonlocal_goto_save_area 
> setup not being such).
> 
> 
> Ciao,
> Michael.
> 

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

[Michael Matz]

Hi,

On Thu, 13 Jul 2017, Martin Liška wrote:

> Hopefully following patch will fix that. I returned to the first version 
> and saved/restored static_chain register before/after 
> __asan_stack_malloc.

It should also work if you emit the parm_birth_note after the static chain 
is set up (not before it), but before you store into the 
nonlocal_goto_save_area.  With that you don't need to worry about 
clobbering the incoming static chain with the asan setup.

Can you test that?  It would better reflect the intent of this note (the 
static chain being an implicit parameter, but the nonlocal_goto_save_area 
setup not being such).


Ciao,
Michael.

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

[Martin Liška]

On 06/30/2017 04:03 PM, Michael Matz wrote:
> 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?

Hi.

Hopefully following patch will fix that. I returned to the first version and
saved/restored static_chain register before/after __asan_stack_malloc.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Thoughts?
Martin
>From b285e7cb1d7f3e35981dec951121db58ce152b3b Mon Sep 17 00:00:00 2001
From: marxin 
Date: Thu, 13 Jul 2017 13:37:47 +0200
Subject: [PATCH] Move static chain and non-local goto init after
 NOTE_INSN_FUNCTION_BEG

gcc/ChangeLog:

2017-06-27  Martin Liska  

PR sanitize/81186
	* function.c (expand_function_start): Move static chain and non-local
	goto init after NOTE_INSN_FUNCTION_BEG.
	* asan.c (asan_emit_stack_protection): Preserve static chain
	register if we call __asan_stack_malloc_N.

gcc/testsuite/ChangeLog:

2017-06-27  Martin Liska  

PR sanitize/81186
	* gcc.dg/asan/pr81186.c: New test.
---
 gcc/asan.c  | 12 
 gcc/function.c  | 18 +-
 gcc/testsuite/gcc.dg/asan/pr81186.c | 18 ++
 3 files changed, 39 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c

diff --git a/gcc/asan.c b/gcc/asan.c
index 89c2731e8cd..9cc1d21c1fb 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1340,6 +1340,16 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
   emit_cmp_and_jump_insns (ret, const0_rtx, EQ, NULL_RTX,
 			   VOIDmode, 0, lab,
 			   profile_probability::very_likely ());
+  /* Preserve static chain register in order to not have it clobbered in
+	 __asan_stack_malloc_N function.  */
+  rtx chain = targetm.calls.static_chain (current_function_decl, true);
+  rtx saved_chain;
+  if (chain)
+	{
+	  saved_chain = gen_reg_rtx (Pmode);
+	  emit_move_insn (saved_chain, chain);
+	}
+
   snprintf (buf, sizeof buf, "__asan_stack_malloc_%d",
 		use_after_return_class);
   ret = init_one_libfunc (buf);
@@ -1347,6 +1357,8 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
  GEN_INT (asan_frame_size
 	  + base_align_bias),
  TYPE_MODE (pointer_sized_int_node));
+  if (chain)
+	emit_move_insn (chain, saved_chain);
   /* __asan_stack_malloc_[n] returns a pointer to fake stack if succeeded
 	 and NULL otherwise.  Check RET value is NULL here and jump over the
 	 BASE reassignment in this case.  Otherwise, reassign BASE to RET.  */
diff --git a/gcc/function.c b/gcc/function.c
index f625489205b..5e8a56099a5 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5220,6 +5220,14 @@ expand_function_start (tree subr)
  In some cases this requires emitting insns.  */
   assign_parms (subr);
 
+  /* The following was moved from init_function_start.
+ The move is supposed to make sdb output more accurate.  */
+  /* Indicate the beginning of the function body,
+ as opposed to parm setup.  */
+  rtx_note *b = emit_note (NOTE_INSN_FUNCTION_BEG);
+
+  gcc_assert (NOTE_P (get_last_insn ()));
+
   /* If function gets a static chain arg, store it.  */
   if (cfun->static_chain_decl)
 {
@@ -5284,15 +5292,7 @@ expand_function_start (tree subr)
   update_nonlocal_goto_save_area ();
 }
 
-  /* The following was moved from init_function_start.
- The move is supposed to make sdb output more accurate.  */
-  /* Indicate the beginning of the function body,
- as opposed to parm setup.  */
-  emit_note (NOTE_INSN_FUNCTION_BEG);
-
-  gcc_assert (NOTE_P (get_last_insn ()));
-
-  parm_birth_insn = get_last_insn ();
+  parm_birth_insn = b;
 
   if (crtl->profile)
 {
diff --git a/gcc/testsuite/gcc.dg/asan/pr81186.c b/gcc/testsuite/gcc.dg/asan/pr81186.c
new file mode 100644
index 000..7f0f672ca40
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/pr81186.c
@@ -0,0 +1,18 @@
+/* PR sanitizer/81186 */
+/* { dg-do run } */
+
+int
+main ()
+{
+  __label__ l;
+  void f ()
+  {
+int a[123];
+
+goto l;
+  }
+
+  f ();
+l:
+  return 0;
+}
-- 
2.13.2

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

[Martin Liška]

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 [0xfff8])) [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]  >>> 

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

[Michael Matz]

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.

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?


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 [0xfff8])) [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]   >> __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 [0xffc0])) [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 [0xffc8])) [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 [0xffa0])))
> >> (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 
> >> 

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

[Martin Liška]

On 06/27/2017 05:29 PM, Michael Matz wrote:
> Hi,
> 
> On Tue, 27 Jun 2017, Martin Liška wrote:
> 
>> Following bug was for me very educative. I learned that we support 
>> non-local gotos that can be combined with nested functions. Real fun :) 
>> Well, the problem is that both cfun->nonlocal_goto_save_area and 
>> cfun->static_chain_decl (emitted in expand_function_start) are put 
>> before NOTE_INSN_FUNCTION_BEG. And so result of expand_used_vars is put 
>> after these instrumentations. That causes problems as it uses stack 
>> before we initialize it (use-after-return checking):
> 
> I don't think that's the right fix.  The purpose of 
> NOTE_INSN_FUNCTION_BEG is to mark the "real" beginning of the function, 
> i.e. without all the compiler generated stuff that's necessary to set up 
> parameters or local variables and so on.  The goto save area and the 
> static chain are also such compiler generated implementation details, and 
> hence are correctly put in front of the function begin note.

Hello.

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.

> 
> 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 [0xfff8])) [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]  > __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 [0xffc0])) [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 [0xffc8])) [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 [0xffa0])))
>> (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]  > 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  
>>
>> PR sanitize/81186
>>  * function.c (expand_function_start): Move static chain and non-local
>>  goto init after NOTE_INSN_FUNCTION_BEG.
>>
>> 

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

[Michael Matz]

Hi,

On Tue, 27 Jun 2017, Martin Liška wrote:

> Following bug was for me very educative. I learned that we support 
> non-local gotos that can be combined with nested functions. Real fun :) 
> Well, the problem is that both cfun->nonlocal_goto_save_area and 
> cfun->static_chain_decl (emitted in expand_function_start) are put 
> before NOTE_INSN_FUNCTION_BEG. And so result of expand_used_vars is put 
> after these instrumentations. That causes problems as it uses stack 
> before we initialize it (use-after-return checking):

I don't think that's the right fix.  The purpose of 
NOTE_INSN_FUNCTION_BEG is to mark the "real" beginning of the function, 
i.e. without all the compiler generated stuff that's necessary to set up 
parameters or local variables and so on.  The goto save area and the 
static chain are also such compiler generated implementation details, and 
hence are correctly put in front of the function begin note.

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 (!).


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 [0xfff8])) [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]   __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 [0xffc0])) [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 [0xffc8])) [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 [0xffa0])))
> (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]   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  
> 
> 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  
> 
> 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
> 
> 
>