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 [0xfffffffffffffff8])) [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] <function_decl 0x2ba005ad5d00
>>>> __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 [0xffffffffffffffc0])) [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 [0xffffffffffffffc8])) [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 [0xffffffffffffffa0])))
>>>> (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] <var_decl
>>>> 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 <mli...@suse.cz>
>>>>
>>>> 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 <mli...@suse.cz>
>>>>
>>>> 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
>>>>
>>>>
>>
