On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
<richard.sandif...@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"

Reply via email to