Hi Richard,
> -----Original Message-----
> From: Richard Sandiford <richard.sandif...@arm.com>
> Sent: 05 August 2020 15:33
> To: gcc-patches@gcc.gnu.org
> Cc: ni...@redhat.com; Richard Earnshaw <richard.earns...@arm.com>;
> Ramana Radhakrishnan <ramana.radhakrish...@arm.com>; Kyrylo
> Tkachov <kyrylo.tkac...@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"
