[ARM] Fix PR85434: 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. This is also known as CVE-2018-12886. 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.

2018-08-02  Thomas Preud'homme  <thomas.preudhomme@linaro.org>

    gcc/
    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.  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/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/
    PR target/85434
    * gcc.target/arm/pr85434.c: New test.

From-SVN: r263245

4 files changed, 136 insertions, 17 deletions

diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index 8537262..100844e 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -67,7 +67,7 @@ extern int const_ok_for_dimode_op (HOST_WIDE_INT, enum rtx_code);
 extern int arm_split_constant (RTX_CODE, machine_mode, rtx,
 			       HOST_WIDE_INT, rtx, rtx, int);
 extern int legitimate_pic_operand_p (rtx);
-extern rtx legitimize_pic_address (rtx, machine_mode, rtx);
+extern rtx legitimize_pic_address (rtx, machine_mode, rtx, rtx, bool);
 extern rtx legitimize_tls_address (rtx, rtx);
 extern bool arm_legitimate_address_p (machine_mode, rtx, bool);
 extern int arm_legitimate_address_outer_p (machine_mode, rtx, RTX_CODE, int);
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index f5eece4..87c728e 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -7369,20 +7369,26 @@ legitimate_pic_operand_p (rtx x)
 }
 
 /* Record that the current function needs a PIC register.  Initialize
-   cfun->machine->pic_reg if we have not already done so.  */
+   cfun->machine->pic_reg if we have not already done so.
+
+   A new pseudo register is used for the PIC register if possible, otherwise
+   PIC_REG must be non NULL and is used instead.  COMPUTE_NOW forces the PIC
+   register to be loaded, irregardless of whether it was loaded previously.  */
 
 static void
-require_pic_register (void)
+require_pic_register (rtx pic_reg, bool compute_now)
 {
   /* A lot of the logic here is made obscure by the fact that this
      routine gets called as part of the rtx cost estimation process.
      We don't want those calls to affect any assumptions about the real
      function; and further, we can't call entry_of_function() until we
      start the real expansion process.  */
-  if (!crtl->uses_pic_offset_table)
+  if (!crtl->uses_pic_offset_table || compute_now)
     {
-      gcc_assert (can_create_pseudo_p ());
+      gcc_assert (can_create_pseudo_p ()
+		  || (pic_reg != NULL_RTX && GET_MODE (pic_reg) == Pmode));
       if (arm_pic_register != INVALID_REGNUM
+	  && can_create_pseudo_p ()
 	  && !(TARGET_THUMB1 && arm_pic_register > LAST_LO_REGNUM))
 	{
 	  if (!cfun->machine->pic_reg)
@@ -7399,7 +7405,8 @@ require_pic_register (void)
 	  rtx_insn *seq, *insn;
 
 	  if (!cfun->machine->pic_reg)
-	    cfun->machine->pic_reg = gen_reg_rtx (Pmode);
+	    cfun->machine->pic_reg =
+	      can_create_pseudo_p () ? gen_reg_rtx (Pmode) : pic_reg;
 
 	  /* Play games to avoid marking the function as needing pic
 	     if we are being called as part of the cost-estimation
@@ -7410,7 +7417,8 @@ require_pic_register (void)
 	      start_sequence ();
 
 	      if (TARGET_THUMB1 && arm_pic_register != INVALID_REGNUM
-		  && arm_pic_register > LAST_LO_REGNUM)
+		  && arm_pic_register > LAST_LO_REGNUM
+		  && can_create_pseudo_p ())
 		emit_move_insn (cfun->machine->pic_reg,
 				gen_rtx_REG (Pmode, arm_pic_register));
 	      else
@@ -7427,15 +7435,29 @@ require_pic_register (void)
 	         we can't yet emit instructions directly in the final
 		 insn stream.  Queue the insns on the entry edge, they will
 		 be committed after everything else is expanded.  */
-	      insert_insn_on_edge (seq,
-				   single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
+	      if (currently_expanding_to_rtl)
+		insert_insn_on_edge (seq,
+				     single_succ_edge
+				     (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
+	      else
+		emit_insn (seq);
 	    }
 	}
     }
 }
 
+/* Legitimize PIC load to ORIG into REG.  If REG is NULL, a new pseudo is
+   created to hold the result of the load.  If not NULL, PIC_REG indicates
+   which register to use as PIC register, otherwise it is decided by register
+   allocator.  COMPUTE_NOW forces the PIC register to be loaded at the current
+   location in the instruction stream, irregardless of whether it was loaded
+   previously.
+
+   Returns the register REG into which the PIC load is performed.  */
+
 rtx
-legitimize_pic_address (rtx orig, machine_mode mode, rtx reg)
+legitimize_pic_address (rtx orig, machine_mode mode, rtx reg, rtx pic_reg,
+			bool compute_now)
 {
   if (GET_CODE (orig) == SYMBOL_REF
       || GET_CODE (orig) == LABEL_REF)
@@ -7469,7 +7491,7 @@ legitimize_pic_address (rtx orig, machine_mode mode, rtx reg)
 	  rtx mem;
 
 	  /* If this function doesn't have a pic register, create one now.  */
-	  require_pic_register ();
+	  require_pic_register (pic_reg, compute_now);
 
 	  pat = gen_calculate_pic_address (reg, cfun->machine->pic_reg, orig);
 
@@ -7520,9 +7542,11 @@ legitimize_pic_address (rtx orig, machine_mode mode, rtx reg)
 
       gcc_assert (GET_CODE (XEXP (orig, 0)) == PLUS);
 
-      base = legitimize_pic_address (XEXP (XEXP (orig, 0), 0), Pmode, reg);
+      base = legitimize_pic_address (XEXP (XEXP (orig, 0), 0), Pmode, reg,
+				     pic_reg, compute_now);
       offset = legitimize_pic_address (XEXP (XEXP (orig, 0), 1), Pmode,
-				       base == reg ? 0 : reg);
+				       base == reg ? 0 : reg, pic_reg,
+				       compute_now);
 
       if (CONST_INT_P (offset))
 	{
@@ -8707,7 +8731,8 @@ arm_legitimize_address (rtx x, rtx orig_x, machine_mode mode)
     {
       /* We need to find and carefully transform any SYMBOL and LABEL
 	 references; so go back to the original address expression.  */
-      rtx new_x = legitimize_pic_address (orig_x, mode, NULL_RTX);
+      rtx new_x = legitimize_pic_address (orig_x, mode, NULL_RTX, NULL_RTX,
+					  false /*compute_now*/);
 
       if (new_x != orig_x)
 	x = new_x;
@@ -8775,7 +8800,8 @@ thumb_legitimize_address (rtx x, rtx orig_x, machine_mode mode)
     {
       /* We need to find and carefully transform any SYMBOL and LABEL
 	 references; so go back to the original address expression.  */
-      rtx new_x = legitimize_pic_address (orig_x, mode, NULL_RTX);
+      rtx new_x = legitimize_pic_address (orig_x, mode, NULL_RTX, NULL_RTX,
+					  false /*compute_now*/);
 
       if (new_x != orig_x)
 	x = new_x;
@@ -18059,7 +18085,7 @@ arm_emit_call_insn (rtx pat, rtx addr, bool sibcall)
 	  ? !targetm.binds_local_p (SYMBOL_REF_DECL (addr))
 	  : !SYMBOL_REF_LOCAL_P (addr)))
     {
-      require_pic_register ();
+      require_pic_register (NULL_RTX, false /*compute_now*/);
       use_reg (&CALL_INSN_FUNCTION_USAGE (insn), cfun->machine->pic_reg);
     }
 
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index ca2a2f5..130531d 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6021,7 +6021,8 @@
       operands[1] = legitimize_pic_address (operands[1], SImode,
 					    (!can_create_pseudo_p ()
 					     ? operands[0]
-					     : 0));
+					     : NULL_RTX), NULL_RTX,
+					    false /*compute_now*/);
   }
   "
 )
@@ -8634,6 +8635,95 @@
    (set_attr "conds" "clob")]
 )
 
+;; 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_SP_SET))
+   (match_scratch:SI 2 "=r")
+   (match_scratch:SI 3 "=r")]
+  ""
+  "#"
+  "reload_completed"
+  [(parallel [(set (match_dup 0) (unspec:SI [(mem:SI (match_dup 2))]
+					    UNSPEC_SP_SET))
+	      (clobber (match_dup 2))])]
+  "
+{
+  rtx addr = XEXP (operands[1], 0);
+  if (flag_pic)
+    {
+      /* Forces recomputing of GOT base now.  */
+      operands[1] = legitimize_pic_address (addr, SImode, operands[2],
+					    operands[3], true /*compute_now*/);
+    }
+  else
+    {
+      if (!address_operand (addr, SImode))
+	operands[1] = force_const_mem (SImode, addr);
+      emit_move_insn (operands[2], operands[1]);
+    }
+}"
+)
+
+(define_insn "stack_protect_set"
+  [(set (match_operand:SI 0 "memory_operand" "=m")
+	(unspec:SI [(mem:SI (match_operand:SI 1 "register_operand" "r"))]
+	 UNSPEC_SP_SET))
+   (clobber (match_dup 1))]
+  ""
+  "ldr\\t%1, [%1]\;str\\t%1, %0\;mov\t%1,0"
+  [(set_attr "length" "12")
+   (set_attr "type" "multiple")])
+
+(define_insn_and_split "stack_protect_combined_test"
+  [(set (pc)
+	(if_then_else
+		(eq (match_operand:SI 0 "memory_operand" "m")
+		    (unspec:SI [(match_operand:SI 1 "memory_operand" "X")]
+			       UNSPEC_SP_TEST))
+		(label_ref (match_operand 2))
+		(pc)))
+   (match_scratch:SI 3 "=r")
+   (match_scratch:SI 4 "=r")]
+  ""
+  "#"
+  "reload_completed"
+  [(const_int 0)]
+{
+  rtx eq, addr;
+
+  addr = XEXP (operands[1], 0);
+  if (flag_pic)
+    {
+      /* Forces recomputing of GOT base now.  */
+      operands[1] = legitimize_pic_address (addr, SImode, operands[3],
+					    operands[4],
+					    true /*compute_now*/);
+    }
+  else
+    {
+      if (!address_operand (addr, SImode))
+	operands[1] = force_const_mem (SImode, addr);
+      emit_move_insn (operands[3], operands[1]);
+    }
+  emit_insn (gen_stack_protect_test (operands[4], operands[0], operands[3]));
+  eq = gen_rtx_EQ (VOIDmode, operands[4], const0_rtx);
+  emit_jump_insn (gen_cbranchsi4 (eq, operands[4], const0_rtx, operands[2]));
+  DONE;
+})
+
+(define_insn "stack_protect_test"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+	(unspec:SI [(match_operand:SI 1 "memory_operand" "m")
+		    (mem:SI (match_operand:SI 2 "register_operand" "r"))]
+	 UNSPEC_SP_TEST))
+   (clobber (match_dup 2))]
+  ""
+  "ldr\t%0, [%2]\;ldr\t%2, %1\;eor\t%0, %2, %0"
+  [(set_attr "length" "12")
+   (set_attr "type" "multiple")])
+
 (define_expand "casesi"
   [(match_operand:SI 0 "s_register_operand" "")	; index to jump on
    (match_operand:SI 1 "const_int_operand" "")	; lower bound
diff --git a/gcc/config/arm/unspecs.md b/gcc/config/arm/unspecs.md
index 1941673..8f9dbcb 100644
--- a/gcc/config/arm/unspecs.md
+++ b/gcc/config/arm/unspecs.md
@@ -86,6 +86,9 @@
   UNSPEC_PROBE_STACK    ; Probe stack memory reference
   UNSPEC_NONSECURE_MEM	; Represent non-secure memory in ARMv8-M with
 			; security extension
+  UNSPEC_SP_SET		; Represent the setting of stack protector's canary
+  UNSPEC_SP_TEST	; Represent the testing of stack protector's canary
+			; against the guard.
 ])
 
 (define_c_enum "unspec" [