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 <thomas.preudho...@linaro.org> 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 > <thomas.preudho...@linaro.org> 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
