Hi,
Please find updated patch to fix PR85434: spilling of stack protector
guard's address on ARM. Quite a few changes have been made to the ARM
part since last round of review so I think it makes more sense to
review it anew. Ran bootstrap + regression testsuite + glibc build +
glibc regression testsuite for Arm and Thumb-2 and bootstrap +
regression testsuite for Thumb-1. GCC's regression testsuite was run
in 3 configurations in all those cases:
- default configuration (no RUNTESTFLAGS)
- with -fstack-protector-all
- with -fPIC -fstack-protector-all (to exercise both codepath in stack
protector's split code)
None of this show any regression beyond some new scan fail with
-fstack-protector-all or -fPIC due to unexpected code sequence for the
testcases concerned and some guality swing due to less optimization
with new stack protector on.
Patch description and ChangeLog below.
In case of high register pressure in PIC mode, address of the stack
protector's guard can be spilled on ARM targets as shown in PR85434,
thus allowing an attacker to control what the canary would be compared
against. ARM does lack stack_protect_set and stack_protect_test insn
patterns, defining them does not help as the address is expanded
regularly and the patterns only deal with the copy and test of the
guard with the canary.
This problem does not occur for x86 targets because the PIC access and
the test can be done in the same instruction. Aarch64 is exempt too
because PIC access insn pattern are mov of UNSPEC which prevents it from
the second access in the epilogue being CSEd in cse_local pass with the
first access in the prologue.
The approach followed here is to create new "combined" set and test
standard pattern names that take the unexpanded guard and do the set or
test. This allows the target to use an opaque pattern (eg. using UNSPEC)
to hide the individual instructions being generated to the compiler and
split the pattern into generic load, compare and branch instruction
after register allocator, therefore avoiding any spilling. This is here
implemented for the ARM targets. For targets not implementing these new
standard pattern names, the existing stack_protect_set and
stack_protect_test pattern names are used.
To be able to split PIC access after register allocation, the functions
had to be augmented to force a new PIC register load and to control
which register it loads into. This is because sharing the PIC register
between prologue and epilogue could lead to spilling due to CSE again
which an attacker could use to control what the canary gets compared
against.
ChangeLog entries are as follows:
*** gcc/ChangeLog ***
2018-10-26 Thomas Preud'homme <thomas.preudhomme@linaro.org>
* target-insns.def (stack_protect_combined_set): Define new standard
pattern name.
(stack_protect_combined_test): Likewise.
* cfgexpand.c (stack_protect_prologue): Try new
stack_protect_combined_set pattern first.
* function.c (stack_protect_epilogue): Try new
stack_protect_combined_test pattern first.
* config/arm/arm.c (require_pic_register): Add pic_reg and compute_now
parameters to control which register to use as PIC register and force
reloading PIC register respectively. Insert in the stream of insns if
possible.
(legitimize_pic_address): Expose above new parameters in prototype and
adapt recursive calls accordingly. Use pic_reg if non null instead of
cached one.
(arm_load_pic_register): Add pic_reg parameter and use it if non null.
(arm_legitimize_address): Adapt to new legitimize_pic_address
prototype.
(thumb_legitimize_address): Likewise.
(arm_emit_call_insn): Adapt to require_pic_register prototype change.
(arm_expand_prologue): Adapt to arm_load_pic_register prototype change.
(thumb1_expand_prologue): Likewise.
* config/arm/arm-protos.h (legitimize_pic_address): Adapt to prototype
change.
(arm_load_pic_register): Likewise.
* config/arm/predicated.md (guard_addr_operand): New predicate.
(guard_operand): New predicate.
* config/arm/arm.md (movsi expander): Adapt to legitimize_pic_address
prototype change.
(builtin_setjmp_receiver expander): Adapt to thumb1_expand_prologue
prototype change.
(stack_protect_combined_set): New expander..
(stack_protect_combined_set_insn): New insn_and_split pattern.
(stack_protect_set_insn): New insn pattern.
(stack_protect_combined_test): New expander.
(stack_protect_combined_test_insn): New insn_and_split pattern.
(arm_stack_protect_test_insn): New insn pattern.
* config/arm/thumb1.md (thumb1_stack_protect_test_insn): New insn pattern.
* config/arm/unspecs.md (UNSPEC_SP_SET): New unspec.
(UNSPEC_SP_TEST): Likewise.
* doc/md.texi (stack_protect_combined_set): Document new standard
pattern name.
(stack_protect_set): Clarify that the operand for guard's address is
legal.
(stack_protect_combined_test): Document new standard pattern name.
(stack_protect_test): Clarify that the operand for guard's address is
legal.
*** gcc/testsuite/ChangeLog ***
2018-07-05 Thomas Preud'homme <thomas.preudhomme@linaro.org>
* gcc.target/arm/pr85434.c: New test.
Is this ok for trunk?
Best regards,
Thomas
On Thu, 25 Oct 2018 at 15:54, Thomas Preudhomme
<thomas.preudhomme@linaro.org> wrote:
Good thing I did, found a missing earlyclobber in the process.
Rerunning all tests again.
Best regards,
Thomas
On Wed, 24 Oct 2018 at 10:13, Thomas Preudhomme
<thomas.preudhomme@linaro.org> wrote:
Please hold on for the reviews, found a small improvement that could
be done. Am testing it right now, should have something by tonight or
tomorrow.
Best regards,
Thomas
On Tue, 23 Oct 2018 at 13:35, Thomas Preudhomme
<thomas.preudhomme@linaro.org> wrote:
[Removing Jeff Law since middle end code hasn't changed]
Hi,
Given how memory operand are reloaded even with an X constraint, I've
reworked the patch for the combined set and combined test instruction
ot keep the mem out of the match_operand and used an expander to
generate the right instruction pattern. I've also fixed some
longstanding issues with the patch when flag_pic is true and with
constraints for Thumb-1 that I hadn't noticed before due to using
dg-cmp-results in conjunction with test_summary which does not show
NA->FAIL (see [1]).
All in all, I think the Arm code would do with a fresh review rather
than looking at the changes since last posted version. (unchanged)
ChangeLog entries are as follows:
*** gcc/ChangeLog ***
2018-08-09 Thomas Preud'homme <thomas.preudhomme@linaro.org>
* target-insns.def (stack_protect_combined_set): Define new standard
pattern name.
(stack_protect_combined_test): Likewise.
* cfgexpand.c (stack_protect_prologue): Try new
stack_protect_combined_set pattern first.
* function.c (stack_protect_epilogue): Try new
stack_protect_combined_test pattern first.
* config/arm/arm.c (require_pic_register): Add pic_reg and compute_now
parameters to control which register to use as PIC register and force
reloading PIC register respectively. Insert in the stream of insns if
possible.
(legitimize_pic_address): Expose above new parameters in prototype and
adapt recursive calls accordingly. Use pic_reg if non null instead of
cached one.
(arm_load_pic_register): Add pic_reg parameter and use it if non null.
(arm_legitimize_address): Adapt to new legitimize_pic_address
prototype.
(thumb_legitimize_address): Likewise.
(arm_emit_call_insn): Adapt to require_pic_register prototype change.
(arm_expand_prologue): Adapt to arm_load_pic_register prototype change.
(thumb1_expand_prologue): Likewise.
* config/arm/arm-protos.h (legitimize_pic_address): Adapt to prototype
change.
(arm_load_pic_register): Likewise.
* config/arm/predicated.md (guard_addr_operand): New predicate.
(guard_operand): New predicate.
* config/arm/arm.md (movsi expander): Adapt to legitimize_pic_address
prototype change.
(builtin_setjmp_receiver expander): Adapt to thumb1_expand_prologue
prototype change.
(stack_protect_combined_set): New expander..
(stack_protect_combined_set_insn): New insn_and_split pattern.
(stack_protect_set_insn): New insn pattern.
(stack_protect_combined_test): New expander.
(stack_protect_combined_test_insn): New insn_and_split pattern.
(stack_protect_test_insn): New insn pattern.
* config/arm/unspecs.md (UNSPEC_SP_SET): New unspec.
(UNSPEC_SP_TEST): Likewise.
* doc/md.texi (stack_protect_combined_set): Document new standard
pattern name.
(stack_protect_set): Clarify that the operand for guard's address is
legal.
(stack_protect_combined_test): Document new standard pattern name.
(stack_protect_test): Clarify that the operand for guard's address is
legal.
*** gcc/testsuite/ChangeLog ***
2018-07-05 Thomas Preud'homme <thomas.preudhomme@linaro.org>
* gcc.target/arm/pr85434.c: New test.
Testing: Bootstrap and regression testing for Arm, Thumb-1 and Thumb-2
with (i) default flags, (ii) an extra -fstack-protect-all and (iii)
-fPIC -fstack-protect-all. A glibc build and testsuite run was also
performed for Arm and Thumb-2. Default flags show no regression and
the other runs have some expected scan-assembler failing (due to stack
protector or fPIC code sequence), as well as guality fail (due to less
optimized code with the new stack protector code) and some execution
failures in sibcall-9 and sibcall-10 under -fPIC -fstack-protector-all
due to the PIC sequence for the global variable making the frame
layout different for the 2 functions (these become PASS if making the
global variable static).
Is this ok for trunk?
Best regards,
Thomas
[1] https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01412.html
On Tue, 25 Sep 2018 at 17:10, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
Hi Thomas,
On 29/08/18 10:51, Thomas Preudhomme wrote:
Resend hopefully without HTML this time.
On Wed, 29 Aug 2018 at 10:49, Thomas Preudhomme
<thomas.preudhomme@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.preudhomme@linaro.org>
* target-insns.def (stack_protect_combined_set): Define new standard
pattern name.
(stack_protect_combined_test): Likewise.
* cfgexpand.c (stack_protect_prologue): Try new
stack_protect_combined_set pattern first.
* function.c (stack_protect_epilogue): Try new
stack_protect_combined_test pattern first.
* config/arm/arm.c (require_pic_register): Add pic_reg and compute_now
parameters to control which register to use as PIC register and force
reloading PIC register respectively. Insert in the stream of insns if
possible.
(legitimize_pic_address): Expose above new parameters in prototype and
adapt recursive calls accordingly.
(arm_legitimize_address): Adapt to new legitimize_pic_address
prototype.
(thumb_legitimize_address): Likewise.
(arm_emit_call_insn): Adapt to new require_pic_register prototype.
* config/arm/arm-protos.h (legitimize_pic_address): Adapt to prototype
change.
* config/arm/predicated.md (guard_operand): New predicate.
Typo, predicates.md is the filename.
Looks ok to me otherwise.
Thank you for your patience.
Kyrill
* config/arm/arm.md (movsi expander): Adapt to legitimize_pic_address
prototype change.
(stack_protect_combined_set): New insn_and_split pattern.
(stack_protect_set): New insn pattern.
(stack_protect_combined_test): New insn_and_split pattern.
(stack_protect_test): New insn pattern.
* config/arm/unspecs.md (UNSPEC_SP_SET): New unspec.
(UNSPEC_SP_TEST): Likewise.
* doc/md.texi (stack_protect_combined_set): Document new standard
pattern name.
(stack_protect_set): Clarify that the operand for guard's address is
legal.
(stack_protect_combined_test): Document new standard pattern name.
(stack_protect_test): Clarify that the operand for guard's address is
legal.
*** gcc/testsuite/ChangeLog ***
2018-07-05 Thomas Preud'homme <thomas.preudhomme@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