Ping? Best regards,
Thomas On Fri, 26 Oct 2018 at 22:41, Thomas Preudhomme <thomas.preudho...@linaro.org> wrote: > > Hi, > > Please find updated patch to fix PR85434: spilling of stack protector > guard's address on ARM. Quite a few changes have been made to the ARM > part since last round of review so I think it makes more sense to > review it anew. Ran bootstrap + regression testsuite + glibc build + > glibc regression testsuite for Arm and Thumb-2 and bootstrap + > regression testsuite for Thumb-1. GCC's regression testsuite was run > in 3 configurations in all those cases: > > - default configuration (no RUNTESTFLAGS) > - with -fstack-protector-all > - with -fPIC -fstack-protector-all (to exercise both codepath in stack > protector's split code) > > None of this show any regression beyond some new scan fail with > -fstack-protector-all or -fPIC due to unexpected code sequence for the > testcases concerned and some guality swing due to less optimization > with new stack protector on. > > Patch description and ChangeLog 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 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 UNSPEC) > 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 here > 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 functions > 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-10-26 Thomas Preud'homme <thomas.preudho...@linaro.org> > > * 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 change. > (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. > (arm_stack_protect_test_insn): New insn pattern. > * config/arm/thumb1.md (thumb1_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 <thomas.preudho...@linaro.org> > > * gcc.target/arm/pr85434.c: New test. > > Is this ok for trunk? > > Best regards, > > Thomas > On Thu, 25 Oct 2018 at 15:54, Thomas Preudhomme > <thomas.preudho...@linaro.org> wrote: > > > > Good thing I did, found a missing earlyclobber in the process. > > Rerunning all tests again. > > > > Best regards, > > > > Thomas > > On Wed, 24 Oct 2018 at 10:13, Thomas Preudhomme > > <thomas.preudho...@linaro.org> wrote: > > > > > > 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 > > > <thomas.preudho...@linaro.org> 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 <thomas.preudho...@linaro.org> > > > > > > > > * 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 > > > > change. > > > > (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 <thomas.preudho...@linaro.org> > > > > > > > > * 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 > > > > <kyrylo.tkac...@foss.arm.com> 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 > > > > > > <thomas.preudho...@linaro.org> 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 unconditionally attempting > > > > > >> to generate the new patterns. The code now tests if there 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 patterns. 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 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 > > > > > >> UNSPEC) > > > > > >> 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 > > > > > >> here > > > > > >> 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 > > > > > >> functions > > > > > >> 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 <thomas.preudho...@linaro.org> > > > > > >> > > > > > >> * 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. > > > > > >> (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 > > > > > >> prototype > > > > > >> 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_address > > > > > >> 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 > > > > > >> address is > > > > > >> legal. > > > > > >> > > > > > >> *** gcc/testsuite/ChangeLog *** > > > > > >> > > > > > >> 2018-07-05 Thomas Preud'homme <thomas.preudho...@linaro.org> > > > > > >> > > > > > >> * gcc.target/arm/pr85434.c: New test. > > > > > > > > > > >> > > > > > >> Testing: > > > > > >> > > > > > >> native x86_64: bootstrap + testsuite -> no regression, can see > > > > > >> failures with previous version of patch but not with new version > > > > > >> native powerpc64: bootstrap + testsuite -> no regression, can see > > > > > >> failures 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 > > > > > >> regression > > > > > >> > > > > > >> Is this ok for trunk? > > > > > >> > > > > > >> Best regards, > > > > > >> > > > > > >> Thomas > > > > >
