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

Kyrylo Tkachov Kyrylo.Tkachov@arm.com
Thu Aug 6 09:12:05 GMT 2020


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"


More information about the Gcc-patches mailing list