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

[Martin Liška <mliska at suse dot cz>]

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