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.
>
>From 13d08eb4c7d1ff7cddd130acad405ec343cb826f Mon Sep 17 00:00:00 2001
From: marxin <mli...@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 <mli...@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 <mli...@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
