Christophe Lyon <christophe.l...@linaro.org> writes: > On Mon, 10 Aug 2020 at 17:27, Richard Sandiford > <richard.sandif...@arm.com> wrote: >> >> Christophe Lyon <christophe.l...@linaro.org> writes: >> > 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) ? >> >> 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 :-) > 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.l...@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. Richard
