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

[Christophe Lyon]

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
-------------- next part --------------
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)