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