Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 * 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 * gcc.target/arm/pr85434.c: New test. Is this o
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 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 > 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 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 > > > > * 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,
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 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 > > * 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 > > >> unconditionally attempting to generate the new patterns. The code now > > >> te
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
[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 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 * 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 > >> 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 co
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 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 * 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 * 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 +
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
On 8/29/18 3:51 AM, 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 >> 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 >> >> * 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. >> * 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 >> >> * 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 ->
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 > > > 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 > > > > > > * 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. > > > * config/arm/arm.md (movsi expander): Adapt to legitimize_pic_address > > > prototype change. > > > (stack_protect_combined_set): Ne
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 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 > > > > * 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. > > * 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. > >
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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 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 > > * 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. > * 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 > > * 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 buil
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
On Tue, Jul 31, 2018 at 6:36 AM, Kyrill Tkachov wrote: > Hi Thomas, > > > On 25/07/18 14:28, Thomas Preudhomme wrote: >> >> Hi Kyrill, >> >> Using memory_operand worked, the issues I encountered when using it in >> earlier versions of the patch must have been due to the missing test >> on address_operand in the preparation statements which I added later. >> Please find an updated patch in attachment. ChangeLog entry is as >> follows: >> >> *** gcc/ChangeLog *** >> >> 2018-07-05 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. >> (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/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 >> >> * gcc.target/arm/pr85434.c: New test. >> >> Bootstrapped again for Arm and Thumb-2 and regtested with and without >> -fstack-protector-all without any regression. > > > This looks ok to me now. > Thank you for your patience and addressing my comments from before. > This breaks x86: FAIL: gcc.dg/fstack-protector-strong.c (internal compiler error) FAIL: gcc.dg/fstack-protector-strong.c (test for excess errors) FAIL: gcc.dg/fstack-protector-strong.c (test for warnings, line 109) FAIL: gcc.dg/pr71585-3.c (internal compiler error) FAIL: gcc.dg/pr71585-3.c (test for excess errors) FAIL: gcc.dg/pr71585.c (internal compiler error) FAIL: gcc.dg/pr71585.c (test for excess errors) FAIL: gcc.target/i386/pr37275.c (internal compiler error) FAIL: gcc.target/i386/pr37275.c (test for excess errors) FAIL: gcc.target/i386/pr47780.c (internal compiler error) FAIL: gcc.target/i386/pr47780.c (test for excess errors) FAIL: gcc.target/i386/pr50788.c (internal compiler error) FAIL: gcc.target/i386/pr50788.c (test for excess errors) FAIL: gcc.target/i386/pr68680.c (internal compiler error) FAIL: gcc.target/i386/pr68680.c (test for excess errors) FAIL: gcc.target/i386/stack-prot-guard.c (internal compiler error) FAIL: gcc.target/i386/stack-prot-guard.c (test for excess errors) FAIL: gcc.target/i386/stack-prot-sym.c (internal compiler error) FAIL: gcc.target/i386/stack-prot-sym.c (test for excess errors) FAIL: g++.dg/fstack-protector-strong.C -std=gnu++11 (internal compiler error) FAIL: g++.dg/fstack-protector-strong.C -std=gnu++11 (test for excess errors) FAIL: g++.dg/fstack-protector-strong.C -std=gnu++14 (internal compiler error) FAIL: g++.dg/fstack-protector-strong.C -std=gnu++14 (test for excess errors) FAIL: g++.dg/fstack-protector-strong.C -std=gnu++98 (internal compiler error) FAIL: g++.dg/fstack-protector-strong.C -std=gnu++98 (test for excess errors) FAIL: g++.dg/pr65032.C (internal compiler error) FAIL: g++.dg/pr65032.C (test for excess errors) FAIL: g++.dg/stackprotectexplicit2.C -std=gnu++11 (internal compiler error) FAIL: g++.dg/stackprotectexplicit2.C -std=gnu++11 (test for excess errors) FAIL: g++.dg/stackprotectexplicit2.C -std=gnu++14 (internal compiler error) FAIL: g++.dg/stackprotectexplicit2.C -std=gnu++14 (test for excess errors) FAIL: g++.dg/stackprotectexplicit2.C -std=gnu++98 (internal compiler error) FAIL: g++.dg/stackprotectexplicit2.C -std=gnu++98 (test for excess errors) -- H.J.
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
Hi Thomas,
On 25/07/18 14:28, Thomas Preudhomme wrote:
Hi Kyrill,
Using memory_operand worked, the issues I encountered when using it in
earlier versions of the patch must have been due to the missing test
on address_operand in the preparation statements which I added later.
Please find an updated patch in attachment. ChangeLog entry is as
follows:
*** gcc/ChangeLog ***
2018-07-05 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.
(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/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
* gcc.target/arm/pr85434.c: New test.
Bootstrapped again for Arm and Thumb-2 and regtested with and without
-fstack-protector-all without any regression.
This looks ok to me now.
Thank you for your patience and addressing my comments from before.
Kyrill
Best regards,
Thomas
On Thu, 19 Jul 2018 at 17:34, Thomas Preudhomme
wrote:
[Dropping Jeff Law from the list since he already commented on the
middle end parts]
Hi Kyrill,
On Thu, 19 Jul 2018 at 12:02, Kyrill Tkachov
wrote:
Hi Thomas,
On 17/07/18 12:02, Thomas Preudhomme wrote:
Fixed in attached patch. ChangeLog entries are unchanged:
*** gcc/ChangeLog ***
2018-07-05 Thomas Preud'homme
PR target/85434
* 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.
(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/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
PR target/85434
* gcc.target/arm/pr85434.c: New test.
Sorry for the delay. Some comments inline.
Kyrill
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d6e3c382085..d1a893ac56e 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6105,8 +6105,18 @@ stack_protect_prologue (void)
{
tree guard_decl = targetm.stack_protect_guard ();
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
Hi Kyrill, Using memory_operand worked, the issues I encountered when using it in earlier versions of the patch must have been due to the missing test on address_operand in the preparation statements which I added later. Please find an updated patch in attachment. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-07-05 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. (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/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 * gcc.target/arm/pr85434.c: New test. Bootstrapped again for Arm and Thumb-2 and regtested with and without -fstack-protector-all without any regression. Best regards, Thomas On Thu, 19 Jul 2018 at 17:34, Thomas Preudhomme wrote: > > [Dropping Jeff Law from the list since he already commented on the > middle end parts] > > Hi Kyrill, > > On Thu, 19 Jul 2018 at 12:02, Kyrill Tkachov > wrote: > > > > Hi Thomas, > > > > On 17/07/18 12:02, Thomas Preudhomme wrote: > > > Fixed in attached patch. ChangeLog entries are unchanged: > > > > > > *** gcc/ChangeLog *** > > > > > > 2018-07-05 Thomas Preud'homme > > > > > > PR target/85434 > > > * 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. > > > (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/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 > > > > > > PR target/85434 > > > * gcc.target/arm/pr85434.c: New test. > > > > > > > Sorry for the delay. Some comments inline. > > > > Kyrill > > > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > > index d6e3c382085..d1a893ac56e 100644 > > --- a/gcc/cfgexpand.c > > +++ b/gcc/cfgexpand.c > > @@ -6
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
[Dropping Jeff Law from the list since he already commented on the
middle end parts]
Hi Kyrill,
On Thu, 19 Jul 2018 at 12:02, Kyrill Tkachov
wrote:
>
> Hi Thomas,
>
> On 17/07/18 12:02, Thomas Preudhomme wrote:
> > Fixed in attached patch. ChangeLog entries are unchanged:
> >
> > *** gcc/ChangeLog ***
> >
> > 2018-07-05 Thomas Preud'homme
> >
> > PR target/85434
> > * 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.
> > (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/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
> >
> > PR target/85434
> > * gcc.target/arm/pr85434.c: New test.
> >
>
> Sorry for the delay. Some comments inline.
>
> Kyrill
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index d6e3c382085..d1a893ac56e 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -6105,8 +6105,18 @@ stack_protect_prologue (void)
> {
> tree guard_decl = targetm.stack_protect_guard ();
> rtx x, y;
> + struct expand_operand ops[2];
>
> x = expand_normal (crtl->stack_protect_guard);
> + create_fixed_operand (&ops[0], x);
> + create_fixed_operand (&ops[1], DECL_RTL (guard_decl));
> + /* Allow the target to compute address of Y and copy it to X without
> + leaking Y into a register. This combined address + copy pattern allows
> + the target to prevent spilling of any intermediate results by splitting
> + it after register allocator. */
> + if (maybe_expand_insn (targetm.code_for_stack_protect_combined_set, 2,
> ops))
> +return;
> +
> if (guard_decl)
> y = expand_normal (guard_decl);
> else
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index 8537262ce64..100844e659c 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -67,7 +67,7 @@ extern int const_ok_for_dimode_op (HOST_WIDE_INT, enum
> rtx_code);
> extern int arm_split_constant (RTX_CODE, machine_mode, rtx,
>HOST_WIDE_INT, rtx, rtx, int);
> extern int legitimate_pic_operand_p (rtx);
> -extern rtx legitimize_pic_address (rtx, machine_mode, rtx);
> +extern rtx legitimize_pic_address (rtx, machine_mode, rtx, rtx, bool);
> extern rtx legitimize_tls_address (rtx, rtx);
> extern bool arm_legitimate_address_p (machine_mode, rtx, bool);
> extern int arm_legitimate_address_outer_p (machine_mode, rtx, RTX_CODE,
> int);
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index ec3abbcba9f..f4a970580c2 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -7369,20 +7369,26 @@ legitimate_pic_operand_p (rtx x)
> }
>
> /* Record that the current function needs a PIC register. Initialize
> - cfun->machine->pic_reg if we have not already done so. */
> + cfun->machine->pic_reg if we have not already done so.
> +
> + If not NULL, PIC_REG indicates which register to use as PIC register,
> + otherwise it is decided by register allocator. COMPUTE_NOW forces the PIC
> + register to be loaded, irregardless of whether it was loaded previously.
> */
>
> static void
> -require_pic_register (void)
> +require_pic_register (rtx pic_reg, bool compute_now)
> {
> /* A lot of the logic here is made obscure by the fact that this
>routine gets called as part of the rtx cost estimation process.
>
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
Hi Thomas,
On 17/07/18 12:02, Thomas Preudhomme wrote:
Fixed in attached patch. ChangeLog entries are unchanged:
*** gcc/ChangeLog ***
2018-07-05 Thomas Preud'homme
PR target/85434
* 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.
(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/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
PR target/85434
* gcc.target/arm/pr85434.c: New test.
Sorry for the delay. Some comments inline.
Kyrill
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d6e3c382085..d1a893ac56e 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6105,8 +6105,18 @@ stack_protect_prologue (void)
{
tree guard_decl = targetm.stack_protect_guard ();
rtx x, y;
+ struct expand_operand ops[2];
x = expand_normal (crtl->stack_protect_guard);
+ create_fixed_operand (&ops[0], x);
+ create_fixed_operand (&ops[1], DECL_RTL (guard_decl));
+ /* Allow the target to compute address of Y and copy it to X without
+ leaking Y into a register. This combined address + copy pattern allows
+ the target to prevent spilling of any intermediate results by splitting
+ it after register allocator. */
+ if (maybe_expand_insn (targetm.code_for_stack_protect_combined_set, 2, ops))
+return;
+
if (guard_decl)
y = expand_normal (guard_decl);
else
diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index 8537262ce64..100844e659c 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -67,7 +67,7 @@ extern int const_ok_for_dimode_op (HOST_WIDE_INT, enum
rtx_code);
extern int arm_split_constant (RTX_CODE, machine_mode, rtx,
HOST_WIDE_INT, rtx, rtx, int);
extern int legitimate_pic_operand_p (rtx);
-extern rtx legitimize_pic_address (rtx, machine_mode, rtx);
+extern rtx legitimize_pic_address (rtx, machine_mode, rtx, rtx, bool);
extern rtx legitimize_tls_address (rtx, rtx);
extern bool arm_legitimate_address_p (machine_mode, rtx, bool);
extern int arm_legitimate_address_outer_p (machine_mode, rtx, RTX_CODE, int);
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index ec3abbcba9f..f4a970580c2 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -7369,20 +7369,26 @@ legitimate_pic_operand_p (rtx x)
}
/* Record that the current function needs a PIC register. Initialize
- cfun->machine->pic_reg if we have not already done so. */
+ cfun->machine->pic_reg if we have not already done so.
+
+ If not NULL, PIC_REG indicates which register to use as PIC register,
+ otherwise it is decided by register allocator. COMPUTE_NOW forces the PIC
+ register to be loaded, irregardless of whether it was loaded previously. */
static void
-require_pic_register (void)
+require_pic_register (rtx pic_reg, bool compute_now)
{
/* A lot of the logic here is made obscure by the fact that this
routine gets called as part of the rtx cost estimation process.
We don't want those calls to affect any assumptions about the real
function; and further, we can't call entry_of_function() until we
start the real expansion process. */
- if (!crtl->uses_pic_offset_table)
+ if (!crtl->uses_pic_offset_table || compute_now)
{
- gcc_assert (can_create_pseudo_p ());
+ gcc_assert (can_create_pseudo_p ()
+ || (pic_reg != NULL_RTX && GET_MODE (pic_reg) == Pmode));
if (arm_pic_register != INVALID_R
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
Fixed in attached patch. ChangeLog entries are unchanged: *** gcc/ChangeLog *** 2018-07-05 Thomas Preud'homme PR target/85434 * 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. (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/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 PR target/85434 * gcc.target/arm/pr85434.c: New test. Best regards, Thomas On Mon, 16 Jul 2018 at 22:46, Jeff Law wrote: > > On 07/05/2018 08:48 AM, Thomas Preudhomme wrote: > > 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-07-05 Thomas Preud'homme > > > > PR target/85434 > > * 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. > > (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. > > * conf
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
On 07/05/2018 08:48 AM, Thomas Preudhomme wrote: > 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-07-05 Thomas Preud'homme > > PR target/85434 > * 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. > (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/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 > > PR target/85434 > * gcc.target/arm/pr85434.c: New test. > > Testing: Bootstrapped on ARM in both Arm and Thumb-2 mode as well as on > Aarch64. Testsuite shows no regression on these 3 variants either both > with default flags and with -fstack-protector-all. > > Is this ok for trunk? If yes, would this be acceptable as a backport to > GCC 6, 7 and 8 provided that no regression is found? > > Best regards, > > Thomas > > > 0001-PR85434-Prevent-spilling-of-stack-protector-guard-s-.patch > > > From d917d48c2005e46154383589f203d06f3c6167e0 Mon Sep 17 00:00:00 2001 > From: Thomas Preud'homme > Date: Tue, 8 May 2018 15:47:05 +0100 > Subject: [PATCH] PR85434: Prevent spilling of stack protector guard's address > on ARM > > 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 > t
Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
Adding Jeff and Eric since the patch adds an RTL target hook. Best regards, Thomas On Thu, 5 Jul 2018 at 15:48, Thomas Preudhomme wrote: > > 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-07-05 Thomas Preud'homme > > PR target/85434 > * 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. > (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/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 > > PR target/85434 > * gcc.target/arm/pr85434.c: New test. > > Testing: Bootstrapped on ARM in both Arm and Thumb-2 mode as well as on > Aarch64. Testsuite shows no regression on these 3 variants either both > with default flags and with -fstack-protector-all. > > Is this ok for trunk? If yes, would this be acceptable as a backport to > GCC 6, 7 and 8 provided that no regression is found? > > Best regards, > > Thomas From d917d48c2005e46154383589f203d06f3c6167e0 Mon Sep 17 00:00:00 2001 From: Thomas Preud'homme Date: Tue, 8 May 2018 15:47:05 +0100 Subject: [PATCH] PR85434: Prevent spilling of stack protector guard's address on ARM 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 sam
[PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM
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-07-05 Thomas Preud'homme PR target/85434 * 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. (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/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 PR target/85434 * gcc.target/arm/pr85434.c: New test. Testing: Bootstrapped on ARM in both Arm and Thumb-2 mode as well as on Aarch64. Testsuite shows no regression on these 3 variants either both with default flags and with -fstack-protector-all. Is this ok for trunk? If yes, would this be acceptable as a backport to GCC 6, 7 and 8 provided that no regression is found? Best regards, Thomas From d917d48c2005e46154383589f203d06f3c6167e0 Mon Sep 17 00:00:00 2001 From: Thomas Preud'homme Date: Tue, 8 May 2018 15:47:05 +0100 Subject: [PATCH] PR85434: Prevent spilling of stack protector guard's address on ARM 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 tha
