From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23263 invoked by alias); 30 Jun 2017 14:33:06 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 22966 invoked by uid 89); 30 Jun 2017 14:33:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-9.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,LOTS_OF_MONEY,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 30 Jun 2017 14:33:03 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id A0AD6AAF2; Fri, 30 Jun 2017 14:33:00 +0000 (UTC) Subject: Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). To: Michael Matz Cc: gcc-patches@gcc.gnu.org, Jakub Jelinek References: <25c189f3-82db-eaa2-8803-2d7ab0727055@suse.cz> <4846559c-7951-6d3e-e4ac-85ec7b8c3c19@suse.cz> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: Date: Fri, 30 Jun 2017 14:33:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg02391.txt.bz2 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] ) [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] ) [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 >>>> >>>> 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 >>>> >>>> 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 >>>> >>>> >>