[gcc r9-8795] arm: Clear canary value after stack_protect_test [PR96191]

[Richard Sandiford]

https://gcc.gnu.org/g:3e40be9cc92d3fa117be7f4fab07cedeed8361a2

commit r9-8795-g3e40be9cc92d3fa117be7f4fab07cedeed8361a2
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Aug 7 12:17:37 2020 +0100

    arm: Clear canary value after stack_protect_test [PR96191]
    
    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.
    
    gcc/
            PR target/96191
            * config/arm/arm.md (arm_stack_protect_test_insn): Zero out
            operand 2 after use.
            * config/arm/thumb1.md (thumb1_stack_protect_test_insn): Likewise.
    
    gcc/testsuite/
            * gcc.target/arm/stack-protector-1.c: New test.
            * gcc.target/arm/stack-protector-2.c: Likewise.
    
    (cherry picked from commit 6a3f3e08723063ea2dadb7ddf503f02972a724e2)

Diff:
---
 gcc/config/arm/arm.md                            |  6 ++-
 gcc/config/arm/thumb1.md                         |  8 +--
 gcc/testsuite/gcc.target/arm/stack-protector-1.c | 63 ++++++++++++++++++++++++
 gcc/testsuite/gcc.target/arm/stack-protector-2.c |  6 +++
 4 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index bfb03f1b96d..aa1f23c8e9e 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -8903,6 +8903,8 @@
   [(set_attr "arch" "t1,32")]
 )
 
+;; DO NOT SPLIT THIS PATTERN.  It is important for security reasons that the
+;; canary value does not live beyond the end of this sequence.
 (define_insn "arm_stack_protect_test_insn"
   [(set (reg:CC_Z CC_REGNUM)
 	(compare:CC_Z (unspec:SI [(match_operand:SI 1 "memory_operand" "m,m")
@@ -8912,8 +8914,8 @@
    (clobber (match_operand:SI 0 "register_operand" "=&l,&r"))
    (clobber (match_dup 2))]
   "TARGET_32BIT"
-  "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0"
-  [(set_attr "length" "8,12")
+  "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;mov\t%2, #0"
+  [(set_attr "length" "12,16")
    (set_attr "conds" "set")
    (set_attr "type" "multiple")
    (set_attr "arch" "t,32")]
diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
index 041e2db343f..6eca8c49c0e 100644
--- a/gcc/config/arm/thumb1.md
+++ b/gcc/config/arm/thumb1.md
@@ -2020,6 +2020,8 @@
   [(set_attr "type" "mov_reg")]
 )
 
+;; DO NOT SPLIT THIS PATTERN.  It is important for security reasons that the
+;; canary value does not live beyond the end of this sequence.
 (define_insn "thumb1_stack_protect_test_insn"
   [(set (match_operand:SI 0 "register_operand" "=&l")
 	(unspec:SI [(match_operand:SI 1 "memory_operand" "m")
@@ -2027,9 +2029,9 @@
 	 UNSPEC_SP_TEST))
    (clobber (match_dup 2))]
   "TARGET_THUMB1"
-  "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0"
-  [(set_attr "length" "8")
-   (set_attr "conds" "set")
+  "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;movs\t%2, #0"
+  [(set_attr "length" "10")
+   (set_attr "conds" "clob")
    (set_attr "type" "multiple")]
 )
 
diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-1.c b/gcc/testsuite/gcc.target/arm/stack-protector-1.c
new file mode 100644
index 00000000000..b03ea14c4e2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-1.c
@@ -0,0 +1,63 @@
+/* { dg-do run } */
+/* { dg-require-effective-target fstack_protector } */
+/* { dg-options "-fstack-protector-all -O2" } */
+
+extern volatile long *stack_chk_guard_ptr;
+
+volatile long *
+get_ptr (void)
+{
+  return stack_chk_guard_ptr;
+}
+
+void __attribute__ ((noipa))
+f (void)
+{
+  volatile int x;
+  x = 1;
+  x += 1;
+}
+
+#define CHECK(REG) "\tcmp\tr0, " #REG "\n\tbeq\t1f\n"
+
+asm (
+"	.data\n"
+"	.align	3\n"
+"	.globl	stack_chk_guard_ptr\n"
+"stack_chk_guard_ptr:\n"
+"	.word	__stack_chk_guard\n"
+"	.weak	__stack_chk_guard\n"
+"__stack_chk_guard:\n"
+"	.word	0xdead4321\n"
+"	.text\n"
+"	.globl	main\n"
+"	.type	main, %function\n"
+"main:\n"
+"	bl	get_ptr\n"
+"	str	r0, [sp, #-8]!\n"
+"	bl	f\n"
+"	str	r0, [sp, #4]\n"
+"	ldr     r0, [sp]\n"
+"	ldr     r0, [r0]\n"
+	CHECK (r1)
+	CHECK (r2)
+	CHECK (r3)
+	CHECK (r4)
+	CHECK (r5)
+	CHECK (r6)
+	CHECK (r7)
+	CHECK (r8)
+	CHECK (r9)
+	CHECK (r10)
+	CHECK (r11)
+	CHECK (r12)
+	CHECK (r13)
+	CHECK (r14)
+"	ldr	r1, [sp, #4]\n"
+	CHECK (r1)
+"	mov	r0, #0\n"
+"	b	exit\n"
+"1:\n"
+"	b	abort\n"
+"	.size	main, .-main"
+);
diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-2.c b/gcc/testsuite/gcc.target/arm/stack-protector-2.c
new file mode 100644
index 00000000000..266c36fdbc6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-2.c
@@ -0,0 +1,6 @@
+/* { dg-do run } */
+/* { dg-require-effective-target fstack_protector } */
+/* { dg-require-effective-target fpic } */
+/* { dg-options "-fstack-protector-all -O2 -fpic" } */
+
+#include "stack-protector-1.c"