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

[Thomas Preudhomme <thomas.preudhomme@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 6279 bytes --]

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

[-- Attachment #2: fix_pr85434_prevent_spilling_stack_protector_guard_address.interdiff --]
[-- Type: application/octet-stream, Size: 5613 bytes --]

diff -u b/gcc/cfgexpand.c b/gcc/cfgexpand.c
--- b/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6106,17 +6106,24 @@
   tree guard_decl = targetm.stack_protect_guard ();
   rtx x, y;
   rtx x, y;
-  struct expand_operand ops[2];
 
   x = expand_normal (crtl->stack_protect_guard);
-  create_fixed_operand (&ops[0], x);
-  create_fixed_operand (&ops[1], DECL_RTL (guard_decl));
-  /* Allow the target to compute address of Y and copy it to X without
-     leaking Y into a register.  This combined address + copy pattern allows
-     the target to prevent spilling of any intermediate results by splitting
-     it after register allocator.  */
-  if (maybe_expand_insn (targetm.code_for_stack_protect_combined_set, 2, ops))
-    return;
+
+  if (targetm.have_stack_protect_combined_set () && guard_decl)
+    {
+      gcc_assert (DECL_P (guard_decl));
+      y = DECL_RTL (guard_decl);
+
+      /* Allow the target to compute address of Y and copy it to X without
+	 leaking Y into a register.  This combined address + copy pattern
+	 allows the target to prevent spilling of any intermediate results by
+	 splitting it after register allocator.  */
+      if (rtx_insn *insn = targetm.gen_stack_protect_combined_set (x, y))
+	{
+	  emit_insn (insn);
+	  return;
+	}
+    }
 
   if (guard_decl)
     y = expand_normal (guard_decl);
diff -u b/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
--- b/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -8638,7 +8638,7 @@
 ;; Named patterns for stack smashing protection.
 (define_insn_and_split "stack_protect_combined_set"
   [(set (match_operand:SI 0 "memory_operand" "=m")
-	(unspec:SI [(match_operand:SI 1 "memory_operand" "X")]
+	(unspec:SI [(match_operand:SI 1 "guard_operand" "X")]
 		   UNSPEC_SP_SET))
    (match_scratch:SI 2 "=r")
    (match_scratch:SI 3 "=r")]
@@ -8659,9 +8659,10 @@
     }
   else
     {
-      if (!address_operand (addr, SImode))
-	operands[1] = force_const_mem (SImode, addr);
-      emit_move_insn (operands[2], operands[1]);
+      if (address_operand (addr, SImode))
+	operands[2] = addr;
+      else
+	operands[2] = XEXP (force_const_mem (SImode, addr), 0);
     }
 }"
 )
@@ -8680,7 +8681,7 @@
   [(set (pc)
 	(if_then_else
 		(eq (match_operand:SI 0 "memory_operand" "m")
-		    (unspec:SI [(match_operand:SI 1 "memory_operand" "X")]
+		    (unspec:SI [(match_operand:SI 1 "guard_operand" "X")]
 			       UNSPEC_SP_TEST))
 		(label_ref (match_operand 2))
 		(pc)))
@@ -8703,9 +8704,10 @@
     }
   else
     {
-      if (!address_operand (addr, SImode))
-	operands[1] = force_const_mem (SImode, addr);
-      emit_move_insn (operands[3], operands[1]);
+      if (address_operand (addr, SImode))
+	operands[3] = addr;
+      else
+	operands[3] = XEXP (force_const_mem (SImode, addr), 0);
     }
   emit_insn (gen_stack_protect_test (operands[4], operands[0], operands[3]));
   eq = gen_rtx_EQ (VOIDmode, operands[4], const0_rtx);
diff -u b/gcc/function.c b/gcc/function.c
--- b/gcc/function.c
+++ b/gcc/function.c
@@ -4892,19 +4892,21 @@
   tree guard_decl = targetm.stack_protect_guard ();
   rtx_code_label *label = gen_label_rtx ();
   rtx x, y;
-  rtx_insn *seq;
-  struct expand_operand ops[3];
+  rtx_insn *seq = 0;
 
   x = expand_normal (crtl->stack_protect_guard);
-  create_fixed_operand (&ops[0], x);
-  create_fixed_operand (&ops[1], DECL_RTL (guard_decl));
-  create_fixed_operand (&ops[2], label);
-  /* Allow the target to compute address of Y and compare it with X without
-     leaking Y into a register.  This combined address + compare pattern allows
-     the target to prevent spilling of any intermediate results by splitting
-     it after register allocator.  */
-  if (!maybe_expand_jump_insn (targetm.code_for_stack_protect_combined_test,
-			       3, ops))
+
+  if (targetm.have_stack_protect_combined_test () && guard_decl)
+    {
+      gcc_assert (DECL_P (guard_decl));
+      y = DECL_RTL (guard_decl);
+      /* Allow the target to compute address of Y and compare it with X without
+	 leaking Y into a register.  This combined address + compare pattern
+	 allows the target to prevent spilling of any intermediate results by
+	 splitting it after register allocator.  */
+      seq = targetm.gen_stack_protect_combined_test (x, y, label);
+    }
+  else
     {
       if (guard_decl)
 	y = expand_normal (guard_decl);
@@ -4914,12 +4916,13 @@
       /* Allow the target to compare Y with X without leaking either into
 	 a register.  */
-      if (targetm.have_stack_protect_test ()
-	  && ((seq = targetm.gen_stack_protect_test (x, y, label))
-	      != NULL_RTX))
-	emit_insn (seq);
-      else
-	emit_cmp_and_jump_insns (x, y, EQ, NULL_RTX, ptr_mode, 1, label);
+      if (targetm.have_stack_protect_test ())
+	seq = targetm.gen_stack_protect_test (x, y, label);
     }
 
+  if (seq)
+    emit_insn (seq);
+  else
+    emit_cmp_and_jump_insns (x, y, EQ, NULL_RTX, ptr_mode, 1, label);
+
   /* The noreturn predictor has been moved to the tree level.  The rtl-level
      predictors estimate this branch about 20%, which isn't enough to get
only in patch2:
unchanged:
--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -31,6 +31,16 @@
 	      || REGNO_REG_CLASS (REGNO (op)) != NO_REGS));
 })
 
+; Predicate for stack protector guard in stack_protect_combined_set and
+; stack_protect_combined_test patterns
+(define_predicate "guard_operand"
+  (match_code "mem")
+{
+  rtx addr = XEXP (op, 0);
+  return (CONSTANT_ADDRESS_P (addr)
+	  || !targetm.cannot_force_const_mem (mode, addr));
+})
+
 (define_predicate "imm_for_neon_inv_logic_operand"
   (match_code "const_vector")
 {