This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, ARM] PR85434: Prevent spilling of stack protector guard's address on ARM


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.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
> 
> 
> 0001-PR85434-Prevent-spilling-of-stack-protector-guard-s-.patch
> 
> 
> From d917d48c2005e46154383589f203d06f3c6167e0 Mon Sep 17 00:00:00 2001
> From: Thomas Preud'homme <thomas.preudhomme@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.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.
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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]