From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68088 invoked by alias); 13 Sep 2018 10:52:30 -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 68078 invoked by uid 89); 13 Sep 2018 10:52:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=1.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SEM_URI,SEM_URIRED,SPF_PASS autolearn=no version=3.3.2 spammy=exempt, attacker, functionc, canary X-HELO: mail-io1-f68.google.com Received: from mail-io1-f68.google.com (HELO mail-io1-f68.google.com) (209.85.166.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Sep 2018 10:52:27 +0000 Received: by mail-io1-f68.google.com with SMTP id y10-v6so2730608ioa.10 for ; Thu, 13 Sep 2018 03:52:26 -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=U4nPk4h5ZTCEXvT0msxCVcWvzWAfZMfgssvXt5Rh5hs=; b=BarjXqxXg8tqpqKWMVUI5xlzY/w7DxYfhhG+GPzJnLC0G+L3geqCtICxP8HweIPGA3 j8FZepImnCf4VoZMjdKttLs5RbRbD4ndLkmzwKaXaFDp9KV5J5dvp8KJXQ4mkqZuXtlf vQfPHYizh4+c5SzX2Z8I4O2ZwoFSpCOkAnaoc= MIME-Version: 1.0 References: In-Reply-To: From: Thomas Preudhomme Date: Thu, 13 Sep 2018 12:02:00 -0000 Message-ID: Subject: Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM To: Jeff Law , kyrylo.tkachov@foss.arm.com, Ramana Radhakrishnan , Richard Earnshaw Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00688.txt.bz2 Hi all, Ping? This new version changes both the middle-end and back-end part so will need a review for both of those. Best regards, Thomas On Wed, 29 Aug 2018 at 11:07, Thomas Preudhomme wrote: > > Forgot another important change in ARM backend: > > The expander were causing one too many indirection which was what > caused the test failure in glibc. The new expanders code skip the > creation of a move from the memory reference of the guard's address to > a register since this is done in the insn themselves. I think during > the initial implementation of the first version of the patch I had > issues with loading the address and used that to load the address. As > can be seen from the absence of regression on the runtime stack > protector test in glibc, this is now working properly, also confirmed > by manual inspection of the code. > > I've attached the interdiff from previous version for reference. > > Best regards, > > Thomas > On Wed, 29 Aug 2018 at 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 g= uard's address on ARM) to address the testsuite regression on powerpc and x= 86 as well as glibc testsuite regression on ARM. Issues were due to uncondi= tionally attempting to generate the new patterns. The code now tests if the= re 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 pa= tterns. 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 compared > > > 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 and > > > 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 f= rom > > > the second access in the epilogue being CSEd in cse_local pass with t= he > > > 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 UNSP= EC) > > > to hide the individual instructions being generated to the compiler a= nd > > > split the pattern into generic load, compare and branch instruction > > > after register allocator, therefore avoiding any spilling. This is he= re > > > implemented for the ARM targets. For targets not implementing these n= ew > > > 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 functio= ns > > > 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 register > > > 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 stand= ard > > > 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 comput= e_now > > > parameters to control which register to use as PIC register and f= orce > > > reloading PIC register respectively. Insert in the stream of ins= ns if > > > possible. > > > (legitimize_pic_address): Expose above new parameters in prototyp= e 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 prototype. > > > * config/arm/arm-protos.h (legitimize_pic_address): Adapt to prot= otype > > > change. > > > * config/arm/predicated.md (guard_operand): New predicate. > > > * config/arm/arm.md (movsi expander): Adapt to legitimize_pic_add= ress > > > 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 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 addres= s 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 failur= es with previous version of patch but not with new version > > > native powerpc64: bootstrap + testsuite -> no regression, can see fai= lures 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 -> n= o regression > > > Aarch64: bootstrap + testsuite + glibc build + glibc test-> no regres= sion > > > > > > Is this ok for trunk? > > > > > > Best regards, > > > > > > Thomas