[PATCH] arm: Clear canary value after stack_protect_test [PR96191]

Richard Sandiford richard.sandiford@arm.com
Wed Aug 5 14:33:09 GMT 2020


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.

Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
I tested the thumb1.md part using arm-linux-gnueabi with the
test flags -march=armv5t -mthumb.  OK for trunk and branches?

As I mentioned in the corresponding aarch64 patch, this is needed
to make arm conform to GCC's current -fstack-protector implementation.
However, I think we should reconsider whether the zeroing is actually
necessary and what it's actually protecting against.  I'll send a
separate message about that to gcc@.  But since the port isn't even
self-consistent (the *set patterns do clear the registers), I think
we should do this first rather than wait for any outcome of that
discussion.

Richard


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.
---
 gcc/config/arm/arm.md                         |  6 +-
 gcc/config/arm/thumb1.md                      |  8 ++-
 .../gcc.target/arm/stack-protector-1.c        | 63 +++++++++++++++++++
 .../gcc.target/arm/stack-protector-2.c        |  6 ++
 4 files changed, 78 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-1.c
 create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-2.c

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index a6a31f8f4ef..dd13c77e889 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -9320,6 +9320,8 @@ (define_insn_and_split "*stack_protect_combined_test_insn"
   [(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")
@@ -9329,8 +9331,8 @@ (define_insn "arm_stack_protect_test_insn"
    (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 24861635fa5..0ff819090d9 100644
--- a/gcc/config/arm/thumb1.md
+++ b/gcc/config/arm/thumb1.md
@@ -2020,6 +2020,8 @@ (define_insn_and_split "thumb_eh_return"
   [(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 @@ (define_insn "thumb1_stack_protect_test_insn"
 	 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"


More information about the Gcc-patches mailing list