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 <thomas.preudho...@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.preudho...@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
>
>
> 0001-PR85434-Prevent-spilling-of-stack-protector-guard-s-.patch
>
>
> From d917d48c2005e46154383589f203d06f3c6167e0 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-07-05 Thomas Preud'homme <thomas.preudho...@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.preudho...@linaro.org>
>
> PR target/85434
> * gcc.target/arm/pr85434.c: New test.
Commenting strictly on the target independent bits... WRT backports,
the release managers would have the final say.
>
> 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
> ---
> gcc/cfgexpand.c | 10 ++
> gcc/config/arm/arm-protos.h | 2 +-
> gcc/config/arm/arm.c | 56 ++++++---
> gcc/config/arm/arm.md | 92 ++++++++++++++-
> gcc/config/arm/unspecs.md | 3 +
> gcc/doc/md.texi | 53 +++++++--
> gcc/function.c | 37 ++++--
> gcc/target-insns.def | 2 +
> gcc/testsuite/gcc.target/arm/pr85434.c | 200
> +++++++++++++++++++++++++++++++++
> 9 files changed, 419 insertions(+), 36 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/arm/pr85434.c
>
> diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
> index 09d6e30..da6423e 100644
> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -7356,22 +7356,61 @@ builtins.
> The get/set patterns have a single output/input operand respectively,
> with @var{mode} intended to be @code{Pmode}.
>
> +@cindex @code{stack_protect_combined_set} instruction pattern
> +@item @samp{stack_protect_combined_set}
> +This pattern, if defined, moves a @code{ptr_mode} value from an address
> +whose declaration RTX is given in operand 1 to the memory in operand 0
> +without leaving the value in a register afterward. If several
> +instructions are needed by the target to perform the operation (eg. to
> +load the address from a GOT entry then load the @code{ptr_mode} value
> +and finallystore it), it is the backend's responsability to ensure no
"finally store" and "responsibility"
> +intermediate result gets spilled. This is to avoid leaking the value
> +some place that an attacker might use to rewrite the stack guard slot
> +after having clobbered it.
> +
> +If this pattern is not defined, then the address declaration is
> +expanded first in the standard way and a @code{stack_protect_set}
> +pattern is then generated to move the value from that address to the
> +address in operand 0.
> +
> @cindex @code{stack_protect_set} instruction pattern
> @item @samp{stack_protect_set}
> -This pattern, if defined, moves a @code{ptr_mode} value from the memory
> -in operand 1 to the memory in operand 0 without leaving the value in
> -a register afterward. This is to avoid leaking the value some place
> -that an attacker might use to rewrite the stack guard slot after
> +This pattern, if defined, moves a @code{ptr_mode} value from the legal
s/legal memory/valid memory location/
> +memory in operand 1 to the memory in operand 0 without leaving the
> +value in a register afterward. This is to avoid leaking the value some
> +place that an attacker might use to rewrite the stack guard slot after
> having clobbered it.
>
> +Note: on targets where the addressing modes do not allow to load
> +directly from stack guard address, the address is expanded in a standard
> +way first which could cause some spills.
> +
> If this pattern is not defined, then a plain move pattern is generated.
>
> +@cindex @code{stack_protect_combined_test} instruction pattern
> +@item @samp{stack_protect_combined_test}
> +This pattern, if defined, compares a @code{ptr_mode} value from an
> +address whose declaration RTX is given in operand 1 with the memory in
> +operand 0 without leaving the value in a register afterward and
> +branches to operand 2 if the values were equal. If several
> +instructions are needed by the target to perform the operation (eg. to
> +load the address from a GOT entry then load the @code{ptr_mode} value
> +and finallystore it), it is the backend's responsability to ensure no
"finally store" and "responsibility" again.
> +intermediate result gets spilled. This is to avoid leaking the value
> +some place that an attacker might use to rewrite the stack guard slot
> +after having clobbered it.
> +
> +If this pattern is not defined, then the address declaration is
> +expanded first in the standard way and a @code{stack_protect_test}
> +pattern is then generated to compare the value from that address to the
> +value at the memory in operand 0.
> +
> @cindex @code{stack_protect_test} instruction pattern
> @item @samp{stack_protect_test}
> This pattern, if defined, compares a @code{ptr_mode} value from the
> -memory in operand 1 with the memory in operand 0 without leaving the
> -value in a register afterward and branches to operand 2 if the values
> -were equal.
> +legal memory in operand 1 with the memory in operand 0 without leaving
s/legal memory/valid memory location/
The rest of the target independent stuff is fine.
jeff
