[PATCH] arm: Clear canary value after stack_protect_test [PR96191]
Tue Aug 11 16:42:44 GMT 2020
Christophe Lyon <email@example.com> writes:
> On Mon, 10 Aug 2020 at 17:27, Richard Sandiford
> <firstname.lastname@example.org> wrote:
>> Christophe Lyon <email@example.com> writes:
>> > On Wed, 5 Aug 2020 at 16:33, Richard Sandiford
>> > <firstname.lastname@example.org> 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, 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 :-)
> 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
> 2020-08-11 Christophe Lyon <email@example.com>
> * gcc.target/arm/stack-protector-1.c: Adapt code to Cortex-M
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.
More information about the Gcc-patches