diff mbox

Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186).

Message ID e12f8746-3293-56d8-c52a-a88094e89826@suse.cz
State New
Headers show

Commit Message

Martin Liška July 13, 2017, 2:15 p.m. UTC 
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

Comments

Michael Matz July 14, 2017, 1:42 p.m. UTC | #1 
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.
Martin Liška July 17, 2017, 12:12 p.m. UTC | #2 
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  0x00000000009850f4 in expand_used_vars () at ../../gcc/cfgexpand.c:2233
#1  0x0000000000992ab7 in (anonymous namespace)::pass_expand::execute (this=0x28c02f0, fun=0x2aaaac1b60b0) at ../../gcc/cfgexpand.c:6232
#2  0x0000000000e0d3a8 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 [0xfffffffffffffdb8])) [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.
>
Michael Matz July 17, 2017, 1:15 p.m. UTC | #3 
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 [0xfffffffffffffdb8])) [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.
diff mbox

Patch

From b285e7cb1d7f3e35981dec951121db58ce152b3b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
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  <mliska@suse.cz>

        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  <mliska@suse.cz>

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