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

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

[Richard Sandiford]

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")]
 )
 \f
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"

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

[Kyrylo Tkachov]

Hi Richard,

> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: 05 August 2020 15:33
> To: gcc-patches@gcc.gnu.org
> Cc: nickc@redhat.com; Richard Earnshaw <Richard.Earnshaw@arm.com>;
> Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com>; Kyrylo
> Tkachov <Kyrylo.Tkachov@arm.com>
> Subject: [PATCH] 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.
> 
> 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.

That makes sense.
Ok.
Thanks,
Kyrill

> 
> 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"

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

[Christophe Lyon]

On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> 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.

Hi Richard,

The new tests fail when compiled with -mcpu=cortex-mXX because gas complains:
use of r13 is deprecated
It has a comment saying: "In the Thumb-2 ISA, use of R13 as Rm is
deprecated, but valid."

It's a minor nuisance, I'm not sure what the best way of getting rid of it?
Add #ifndef __thumb2__ around CHECK(r13) ?

Christophe

> ---
>  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"

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

[Richard Sandiford]

Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> 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.
>
> Hi Richard,
>
> The new tests fail when compiled with -mcpu=cortex-mXX because gas complains:
> use of r13 is deprecated
> It has a comment saying: "In the Thumb-2 ISA, use of R13 as Rm is
> deprecated, but valid."
>
> It's a minor nuisance, I'm not sure what the best way of getting rid of it?
> Add #ifndef __thumb2__ around CHECK(r13) ?

Hmm, maybe we should just drop that line altogether.  It wasn't exactly
likely that r13 would be the register to leak the value :-)

Should I post a patch or do you already have one ready?

Thanks,
Richard

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

[Christophe Lyon]

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

On Mon, 10 Aug 2020 at 17:27, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> > On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> 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.
> >
> > Hi Richard,
> >
> > The new tests fail when compiled with -mcpu=cortex-mXX because gas complains:
> > use of r13 is deprecated
> > It has a comment saying: "In the Thumb-2 ISA, use of R13 as Rm is
> > deprecated, but valid."
> >
> > It's a minor nuisance, I'm not sure what the best way of getting rid of it?
> > Add #ifndef __thumb2__ around CHECK(r13) ?
>
> Hmm, maybe we should just drop that line altogether.  It wasn't exactly
> likely that r13 would be the register to leak the value :-)
>
> Should I post a patch or do you already have one ready?

I was about to push the patch that removes the line CHECK(r13).

However, I've noticed that when using -mcpu=cortex-m[01], we have an
error from gas:
Error: Thumb does not support this addressing mode -- `str r0,[sp,#-8]!'

The attached patch replaces this instruction with
    sub sp,sp,8
    str r0,[rp]

Checked with cortex-m0 and cortex-m3.

OK?

Thanks,

Christophe


>
> Thanks,
> Richard

[-- Attachment #2: stack-protector-test.patch.txt --]
[-- Type: text/plain, Size: 1163 bytes --]

testsuite: Fix gcc.target/arm/stack-protector-1.c for Cortex-M

The stack-protector-1.c test fails when compiled for Cortex-M:
- for Cortex-M0/M1, str r0, [sp #-8]! is not supported
- for Cortex-M3/M4..., the assembler complains that "use of r13 is
  deprecated"

This patch replaces the str instruction with
     sub   sp, sp, #8
     str r0, [sp]
and removes the check for r13, which is unlikely to leak the canary
value.

2020-08-11  Christophe Lyon  <christophe.lyon@linaro.org>

	gcc/testsuite/
	* gcc.target/arm/stack-protector-1.c: Adapt code to Cortex-M
	restrictions.

diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-1.c b/gcc/testsuite/gcc.target/arm/stack-protector-1.c
index b03ea14..8d28b0a 100644
--- a/gcc/testsuite/gcc.target/arm/stack-protector-1.c
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-1.c
@@ -34,7 +34,8 @@ asm (
 "	.type	main, %function\n"
 "main:\n"
 "	bl	get_ptr\n"
-"	str	r0, [sp, #-8]!\n"
+"	sub	sp, sp, #8\n"
+"	str	r0, [sp]\n"
 "	bl	f\n"
 "	str	r0, [sp, #4]\n"
 "	ldr     r0, [sp]\n"
@@ -51,7 +52,6 @@ asm (
 	CHECK (r10)
 	CHECK (r11)
 	CHECK (r12)
-	CHECK (r13)
 	CHECK (r14)
 "	ldr	r1, [sp, #4]\n"
 	CHECK (r1)

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

[Richard Sandiford]

Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Mon, 10 Aug 2020 at 17:27, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Christophe Lyon <christophe.lyon@linaro.org> writes:
>> > On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
>> > <richard.sandiford@arm.com> wrote:
>> >>
>> >> 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.
>> >
>> > Hi Richard,
>> >
>> > The new tests fail when compiled with -mcpu=cortex-mXX because gas complains:
>> > use of r13 is deprecated
>> > It has a comment saying: "In the Thumb-2 ISA, use of R13 as Rm is
>> > deprecated, but valid."
>> >
>> > It's a minor nuisance, I'm not sure what the best way of getting rid of it?
>> > Add #ifndef __thumb2__ around CHECK(r13) ?
>>
>> Hmm, maybe we should just drop that line altogether.  It wasn't exactly
>> likely that r13 would be the register to leak the value :-)
>>
>> Should I post a patch or do you already have one ready?
>
> I was about to push the patch that removes the line CHECK(r13).
>
> However, I've noticed that when using -mcpu=cortex-m[01], we have an
> error from gas:
> Error: Thumb does not support this addressing mode -- `str r0,[sp,#-8]!'

Seems like writing a correct arm.exp test is almost as difficult
(for me) as writing a correct vect.exp test :-)

> This patch replaces the str instruction with
>      sub   sp, sp, #8
>      str r0, [sp]
> and removes the check for r13, which is unlikely to leak the canary
> value.
>
> 2020-08-11  Christophe Lyon  <christophe.lyon@linaro.org>
>
> 	gcc/testsuite/
> 	* gcc.target/arm/stack-protector-1.c: Adapt code to Cortex-M
> 	restrictions.

OK, thanks.  I'm afraid this is already on GCC 10 and 9, so OK there too.
I'll fold this in when backporting to GCC 8.

Richard

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

[Christophe Lyon]

On Tue, 11 Aug 2020 at 18:42, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> > On Mon, 10 Aug 2020 at 17:27, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Christophe Lyon <christophe.lyon@linaro.org> writes:
> >> > On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
> >> > <richard.sandiford@arm.com> wrote:
> >> >>
> >> >> 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.
> >> >
> >> > Hi Richard,
> >> >
> >> > The new tests fail when compiled with -mcpu=cortex-mXX because gas complains:
> >> > use of r13 is deprecated
> >> > It has a comment saying: "In the Thumb-2 ISA, use of R13 as Rm is
> >> > deprecated, but valid."
> >> >
> >> > It's a minor nuisance, I'm not sure what the best way of getting rid of it?
> >> > Add #ifndef __thumb2__ around CHECK(r13) ?
> >>
> >> Hmm, maybe we should just drop that line altogether.  It wasn't exactly
> >> likely that r13 would be the register to leak the value :-)
> >>
> >> Should I post a patch or do you already have one ready?
> >
> > I was about to push the patch that removes the line CHECK(r13).
> >
> > However, I've noticed that when using -mcpu=cortex-m[01], we have an
> > error from gas:
> > Error: Thumb does not support this addressing mode -- `str r0,[sp,#-8]!'
>
> Seems like writing a correct arm.exp test is almost as difficult
> (for me) as writing a correct vect.exp test :-)

:-) Yeah, there are way too many combinations


> > This patch replaces the str instruction with
> >      sub   sp, sp, #8
> >      str r0, [sp]
> > and removes the check for r13, which is unlikely to leak the canary
> > value.
> >
> > 2020-08-11  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> >       gcc/testsuite/
> >       * gcc.target/arm/stack-protector-1.c: Adapt code to Cortex-M
> >       restrictions.
>
> OK, thanks.  I'm afraid this is already on GCC 10 and 9, so OK there too.
> I'll fold this in when backporting to GCC 8.
>
Thanks, pushed to master, gcc-9 and gcc-10.

> Richard