On Tue, 11 Aug 2020 at 18:42, Richard Sandiford <richard.sandif...@arm.com> wrote: > > 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 :-)
:-) 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.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. > Thanks, pushed to master, gcc-9 and gcc-10. > Richard
