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 > <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. >> * 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 > > fix_pr85434_prevent_spilling_stack_protector_guard_address.patch > > From 922cc5d7054bc598732e4ad6d408c7e4297c519a Mon Sep 17 00:00:00 2001 > From: Thomas Preud'homme <thomas.preudho...@linaro.org> > 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 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. > * 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: 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 > > Change-Id: I993343e3063fb570af706624e08b475732a5ec57 > --- > gcc/cfgexpand.c | 17 +++ > gcc/config/arm/arm-protos.h | 2 +- > gcc/config/arm/arm.c | 56 +++++-- > gcc/config/arm/arm.md | 94 +++++++++++- > gcc/config/arm/predicates.md | 10 ++ > gcc/config/arm/unspecs.md | 3 + > gcc/doc/md.texi | 55 ++++++- > gcc/function.c | 32 +++- > gcc/target-insns.def | 2 + > gcc/testsuite/gcc.target/arm/pr85434.c | 200 +++++++++++++++++++++++++ > 10 files changed, 438 insertions(+), 33 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/arm/pr85434.c I think the target independent bits here are still fine.
Jeff
