From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84914 invoked by alias); 30 Jun 2017 14:03:56 -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 83901 invoked by uid 89); 30 Jun 2017 14:03:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=slots 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:03:53 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 3538EAC02; Fri, 30 Jun 2017 14:03:51 +0000 (UTC) Date: Fri, 30 Jun 2017 14:03:00 -0000 From: Michael Matz To: =?ISO-8859-15?Q?Martin_Li=A8ka?= cc: gcc-patches@gcc.gnu.org, Jakub Jelinek Subject: Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). In-Reply-To: <4846559c-7951-6d3e-e4ac-85ec7b8c3c19@suse.cz> Message-ID: References: <25c189f3-82db-eaa2-8803-2d7ab0727055@suse.cz> <4846559c-7951-6d3e-e4ac-85ec7b8c3c19@suse.cz> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="-1609957120-1223736088-1498830105=:12691" Content-ID: X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg02387.txt.bz2 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1609957120-1223736088-1498830105=:12691 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: Content-length: 6139 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. 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? 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 > >> > >> > > ---1609957120-1223736088-1498830105=:12691--