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

Christophe Lyon christophe.lyon@linaro.org
Wed Aug 12 09:26:55 GMT 2020


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


More information about the Gcc-patches mailing list