From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94519 invoked by alias); 24 Oct 2018 09:14:16 -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 94506 invoked by uid 89); 24 Oct 2018 09:14:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Prevent X-HELO: mail-io1-f65.google.com Received: from mail-io1-f65.google.com (HELO mail-io1-f65.google.com) (209.85.166.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Oct 2018 09:14:13 +0000 Received: by mail-io1-f65.google.com with SMTP id y10-v6so2676429ioa.10 for ; Wed, 24 Oct 2018 02:14:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=9fqVy4afeqebxG/Fedfsk1yghC3l3Y8NRttRlO1Vnb0=; b=ToulKUFKRQiRQv77MWRibWVsgbpQcAaoSAe1HfYoWaAQ4UQUtp2pmhTj/2QDrk6WlM TRJdgAu+bF258Jbi5IJiWjN+7LBC5O5YZTLBgiqtmYECiSgqSmA3YMuYG5R0v0WFPKPa cQ64QLAJjYT++Pjrp/6f1V1tIjXVoHDcW3uVI= MIME-Version: 1.0 References: <5BAA5DD7.1070502@foss.arm.com> In-Reply-To: From: Thomas Preudhomme Date: Wed, 24 Oct 2018 10:38:00 -0000 Message-ID: Subject: Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM To: kyrylo.tkachov@foss.arm.com Cc: Ramana Radhakrishnan , Richard Earnshaw , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg01492.txt.bz2 Please hold on for the reviews, found a small improvement that could be done. Am testing it right now, should have something by tonight or tomorrow. Best regards, Thomas On Tue, 23 Oct 2018 at 13:35, Thomas Preudhomme wrote: > > [Removing Jeff Law since middle end code hasn't changed] > > Hi, > > Given how memory operand are reloaded even with an X constraint, I've > reworked the patch for the combined set and combined test instruction > ot keep the mem out of the match_operand and used an expander to > generate the right instruction pattern. I've also fixed some > longstanding issues with the patch when flag_pic is true and with > constraints for Thumb-1 that I hadn't noticed before due to using > dg-cmp-results in conjunction with test_summary which does not show > NA->FAIL (see [1]). > > All in all, I think the Arm code would do with a fresh review rather > than looking at the changes since last posted version. (unchanged) > ChangeLog entries are as follows: > > *** gcc/ChangeLog *** > > 2018-08-09 Thomas Preud'homme > > * target-insns.def (stack_protect_combined_set): Define new standard > pattern name. > (stack_protect_combined_test): Likewise. > * cfgexpand.c (stack_protect_prologue): Try new > stack_protect_combined_set pattern first. > * function.c (stack_protect_epilogue): Try new > stack_protect_combined_test pattern first. > * config/arm/arm.c (require_pic_register): Add pic_reg and compute_now > parameters to control which register to use as PIC register and force > reloading PIC register respectively. Insert in the stream of insns if > possible. > (legitimize_pic_address): Expose above new parameters in prototype and > adapt recursive calls accordingly. Use pic_reg if non null instead of > cached one. > (arm_load_pic_register): Add pic_reg parameter and use it if non null. > (arm_legitimize_address): Adapt to new legitimize_pic_address > prototype. > (thumb_legitimize_address): Likewise. > (arm_emit_call_insn): Adapt to require_pic_register prototype change. > (arm_expand_prologue): Adapt to arm_load_pic_register prototype chang= e. > (thumb1_expand_prologue): Likewise. > * config/arm/arm-protos.h (legitimize_pic_address): Adapt to prototype > change. > (arm_load_pic_register): Likewise. > * config/arm/predicated.md (guard_addr_operand): New predicate. > (guard_operand): New predicate. > * config/arm/arm.md (movsi expander): Adapt to legitimize_pic_address > prototype change. > (builtin_setjmp_receiver expander): Adapt to thumb1_expand_prologue > prototype change. > (stack_protect_combined_set): New expander.. > (stack_protect_combined_set_insn): New insn_and_split pattern. > (stack_protect_set_insn): New insn pattern. > (stack_protect_combined_test): New expander. > (stack_protect_combined_test_insn): New insn_and_split pattern. > (stack_protect_test_insn): New insn pattern. > * config/arm/unspecs.md (UNSPEC_SP_SET): New unspec. > (UNSPEC_SP_TEST): Likewise. > * doc/md.texi (stack_protect_combined_set): Document new standard > pattern name. > (stack_protect_set): Clarify that the operand for guard's address is > legal. > (stack_protect_combined_test): Document new standard pattern name. > (stack_protect_test): Clarify that the operand for guard's address is > legal. > > *** gcc/testsuite/ChangeLog *** > > 2018-07-05 Thomas Preud'homme > > * gcc.target/arm/pr85434.c: New test. > > Testing: Bootstrap and regression testing for Arm, Thumb-1 and Thumb-2 > with (i) default flags, (ii) an extra -fstack-protect-all and (iii) > -fPIC -fstack-protect-all. A glibc build and testsuite run was also > performed for Arm and Thumb-2. Default flags show no regression and > the other runs have some expected scan-assembler failing (due to stack > protector or fPIC code sequence), as well as guality fail (due to less > optimized code with the new stack protector code) and some execution > failures in sibcall-9 and sibcall-10 under -fPIC -fstack-protector-all > due to the PIC sequence for the global variable making the frame > layout different for the 2 functions (these become PASS if making the > global variable static). > > Is this ok for trunk? > > Best regards, > > Thomas > > [1] https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01412.html > > > On Tue, 25 Sep 2018 at 17:10, Kyrill Tkachov > wrote: > > > > Hi Thomas, > > > > On 29/08/18 10:51, Thomas Preudhomme wrote: > > > Resend hopefully without HTML this time. > > > > > > On Wed, 29 Aug 2018 at 10:49, Thomas Preudhomme > > > wrote: > > >> Hi, > > >> > > >> I've reworked the patch fixing PR85434 (spilling of stack protector = guard's address on ARM) to address the testsuite regression on powerpc and = x86 as well as glibc testsuite regression on ARM. Issues were due to uncond= itionally attempting to generate the new patterns. The code now tests if th= ere is a pattern for them for the target before generating them. In the ARM= side of the patch, I've also added a more specific predicate for the new p= atterns. The new patch is found below. > > >> > > >> > > >> In case of high register pressure in PIC mode, address of the stack > > >> protector's guard can be spilled on ARM targets as shown in PR85434, > > >> thus allowing an attacker to control what the canary would be compar= ed > > >> against. ARM does lack stack_protect_set and stack_protect_test insn > > >> patterns, defining them does not help as the address is expanded > > >> regularly and the patterns only deal with the copy and test of the > > >> guard with the canary. > > >> > > >> This problem does not occur for x86 targets because the PIC access a= nd > > >> the test can be done in the same instruction. Aarch64 is exempt too > > >> because PIC access insn pattern are mov of UNSPEC which prevents it = from > > >> the second access in the epilogue being CSEd in cse_local pass with = the > > >> first access in the prologue. > > >> > > >> The approach followed here is to create new "combined" set and test > > >> standard pattern names that take the unexpanded guard and do the set= or > > >> test. This allows the target to use an opaque pattern (eg. using UNS= PEC) > > >> to hide the individual instructions being generated to the compiler = and > > >> split the pattern into generic load, compare and branch instruction > > >> after register allocator, therefore avoiding any spilling. This is h= ere > > >> implemented for the ARM targets. For targets not implementing these = new > > >> standard pattern names, the existing stack_protect_set and > > >> stack_protect_test pattern names are used. > > >> > > >> To be able to split PIC access after register allocation, the functi= ons > > >> had to be augmented to force a new PIC register load and to control > > >> which register it loads into. This is because sharing the PIC regist= er > > >> between prologue and epilogue could lead to spilling due to CSE again > > >> which an attacker could use to control what the canary gets compared > > >> against. > > >> > > >> ChangeLog entries are as follows: > > >> > > >> *** gcc/ChangeLog *** > > >> > > >> 2018-08-09 Thomas Preud'homme > > >> > > >> * target-insns.def (stack_protect_combined_set): Define new sta= ndard > > >> pattern name. > > >> (stack_protect_combined_test): Likewise. > > >> * cfgexpand.c (stack_protect_prologue): Try new > > >> stack_protect_combined_set pattern first. > > >> * function.c (stack_protect_epilogue): Try new > > >> stack_protect_combined_test pattern first. > > >> * config/arm/arm.c (require_pic_register): Add pic_reg and comp= ute_now > > >> parameters to control which register to use as PIC register and= force > > >> reloading PIC register respectively. Insert in the stream of i= nsns if > > >> possible. > > >> (legitimize_pic_address): Expose above new parameters in protot= ype and > > >> adapt recursive calls accordingly. > > >> (arm_legitimize_address): Adapt to new legitimize_pic_address > > >> prototype. > > >> (thumb_legitimize_address): Likewise. > > >> (arm_emit_call_insn): Adapt to new require_pic_register prototy= pe. > > >> * config/arm/arm-protos.h (legitimize_pic_address): Adapt to pr= ototype > > >> change. > > >> * config/arm/predicated.md (guard_operand): New predicate. > > > > Typo, predicates.md is the filename. > > > > Looks ok to me otherwise. > > Thank you for your patience. > > > > Kyrill > > > > >> * config/arm/arm.md (movsi expander): Adapt to legitimize_pic_a= ddress > > >> prototype change. > > >> (stack_protect_combined_set): New insn_and_split pattern. > > >> (stack_protect_set): New insn pattern. > > >> (stack_protect_combined_test): New insn_and_split pattern. > > >> (stack_protect_test): New insn pattern. > > >> * config/arm/unspecs.md (UNSPEC_SP_SET): New unspec. > > >> (UNSPEC_SP_TEST): Likewise. > > >> * doc/md.texi (stack_protect_combined_set): Document new standa= rd > > >> pattern name. > > >> (stack_protect_set): Clarify that the operand for guard's addre= ss is > > >> legal. > > >> (stack_protect_combined_test): Document new standard pattern na= me. > > >> (stack_protect_test): Clarify that the operand for guard's addr= ess is > > >> legal. > > >> > > >> *** gcc/testsuite/ChangeLog *** > > >> > > >> 2018-07-05 Thomas Preud'homme > > >> > > >> * gcc.target/arm/pr85434.c: New test. > > > > >> > > >> Testing: > > >> > > >> native x86_64: bootstrap + testsuite -> no regression, can see failu= res with previous version of patch but not with new version > > >> native powerpc64: bootstrap + testsuite -> no regression, can see fa= ilures from pr86834 with previous version of patch but not with new version > > >> cross ARM Linux: build + testsuite -> no regression > > >> native ARM Thumb-2: bootstrap + testsuite + glibc build + glibc test= -> no regression > > >> native ARM Arm: bootstrap + testsuite + glibc build + glibc test -> = no regression > > >> Aarch64: bootstrap + testsuite + glibc build + glibc test-> no regre= ssion > > >> > > >> Is this ok for trunk? > > >> > > >> Best regards, > > >> > > >> Thomas > >