| Message ID | 99d1bcb3-5213-d21d-d1ee-205c1420146f@suse.cz |
|---|---|
| State | New |
| Headers | show |
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 [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. > > 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 [0xfffffffffffffff8])) [0 S8 A64]) > (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1 > (nil)) > > is just storage of &FRAME.0 from caller where content of the FRAME struct lives on stack (and thus on > shadow stack). That said it's perfectly fine to store &CHAIN 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]; > *(&buffer[0] - 4) = 123; > > goto l; > } > > f (); > l: > return 0; > } > > It's funny that *(&buffer[0] - 4) actually corrupts __nl_goto_buf and we end up with a > dead signal: > > ASAN:DEADLYSIGNAL > ================================================================= > ==30888==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x000000000000 sp 0x7ffe0000049b 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. >> >
On Tue, Jul 18, 2017 at 10:38:50AM +0200, Martin Liška wrote: > 2017-06-27 Martin Liska <mliska@suse.cz> > > 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
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 <mliska@suse.cz> >> >> 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 >
From 13d08eb4c7d1ff7cddd130acad405ec343cb826f 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): Set parm_birth_insn after
static chain is initialized.
gcc/testsuite/ChangeLog:
2017-06-27 Martin Liska <mliska@suse.cz>
PR sanitize/81186
* gcc.dg/asan/pr81186.c: New test.
---
gcc/function.c | 20 ++++++++++----------
gcc/testsuite/gcc.dg/asan/pr81186.c | 18 ++++++++++++++++++
2 files changed, 28 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c
diff --git a/gcc/function.c b/gcc/function.c
index f625489205b..9cfe58afe90 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5263,6 +5263,16 @@ expand_function_start (tree 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. */
+ emit_note (NOTE_INSN_FUNCTION_BEG);
+
+ gcc_assert (NOTE_P (get_last_insn ()));
+
+ parm_birth_insn = get_last_insn ();
+
/* If the function receives a non-local goto, then store the
bits we need to restore the frame pointer. */
if (cfun->nonlocal_goto_save_area)
@@ -5284,16 +5294,6 @@ 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 ();
-
if (crtl->profile)
{
#ifdef PROFILE_HOOK
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