| Message ID | mptzh792me2.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Clear canary value after stack_protect_test [PR96191] | expand |
Hi Richard, > -----Original Message----- > From: Richard Sandiford <richard.sandiford@arm.com> > Sent: 05 August 2020 15:33 > To: gcc-patches@gcc.gnu.org > Cc: nickc@redhat.com; Richard Earnshaw <Richard.Earnshaw@arm.com>; > Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com>; Kyrylo > Tkachov <Kyrylo.Tkachov@arm.com> > Subject: [PATCH] arm: Clear canary value after stack_protect_test [PR96191] > > 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. That makes sense. Ok. Thanks, Kyrill > > 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. > --- > gcc/config/arm/arm.md | 6 +- > gcc/config/arm/thumb1.md | 8 ++- > .../gcc.target/arm/stack-protector-1.c | 63 +++++++++++++++++++ > .../gcc.target/arm/stack-protector-2.c | 6 ++ > 4 files changed, 78 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-1.c > create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-2.c > > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md > index a6a31f8f4ef..dd13c77e889 100644 > --- a/gcc/config/arm/arm.md > +++ b/gcc/config/arm/arm.md > @@ -9320,6 +9320,8 @@ (define_insn_and_split > "*stack_protect_combined_test_insn" > [(set_attr "arch" "t1,32")] > ) > > +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the > +;; canary value does not live beyond the end of this sequence. > (define_insn "arm_stack_protect_test_insn" > [(set (reg:CC_Z CC_REGNUM) > (compare:CC_Z (unspec:SI [(match_operand:SI 1 "memory_operand" > "m,m") > @@ -9329,8 +9331,8 @@ (define_insn "arm_stack_protect_test_insn" > (clobber (match_operand:SI 0 "register_operand" "=&l,&r")) > (clobber (match_dup 2))] > "TARGET_32BIT" > - "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0" > - [(set_attr "length" "8,12") > + "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;mov\t%2, #0" > + [(set_attr "length" "12,16") > (set_attr "conds" "set") > (set_attr "type" "multiple") > (set_attr "arch" "t,32")] > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md > index 24861635fa5..0ff819090d9 100644 > --- a/gcc/config/arm/thumb1.md > +++ b/gcc/config/arm/thumb1.md > @@ -2020,6 +2020,8 @@ (define_insn_and_split "thumb_eh_return" > [(set_attr "type" "mov_reg")] > ) > > +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the > +;; canary value does not live beyond the end of this sequence. > (define_insn "thumb1_stack_protect_test_insn" > [(set (match_operand:SI 0 "register_operand" "=&l") > (unspec:SI [(match_operand:SI 1 "memory_operand" "m") > @@ -2027,9 +2029,9 @@ (define_insn "thumb1_stack_protect_test_insn" > UNSPEC_SP_TEST)) > (clobber (match_dup 2))] > "TARGET_THUMB1" > - "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0" > - [(set_attr "length" "8") > - (set_attr "conds" "set") > + "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;movs\t%2, #0" > + [(set_attr "length" "10") > + (set_attr "conds" "clob") > (set_attr "type" "multiple")] > ) > > > > diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-1.c > b/gcc/testsuite/gcc.target/arm/stack-protector-1.c > new file mode 100644 > index 00000000000..b03ea14c4e2 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/stack-protector-1.c > @@ -0,0 +1,63 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target fstack_protector } */ > +/* { dg-options "-fstack-protector-all -O2" } */ > + > +extern volatile long *stack_chk_guard_ptr; > + > +volatile long * > +get_ptr (void) > +{ > + return stack_chk_guard_ptr; > +} > + > +void __attribute__ ((noipa)) > +f (void) > +{ > + volatile int x; > + x = 1; > + x += 1; > +} > + > +#define CHECK(REG) "\tcmp\tr0, " #REG "\n\tbeq\t1f\n" > + > +asm ( > +" .data\n" > +" .align 3\n" > +" .globl stack_chk_guard_ptr\n" > +"stack_chk_guard_ptr:\n" > +" .word __stack_chk_guard\n" > +" .weak __stack_chk_guard\n" > +"__stack_chk_guard:\n" > +" .word 0xdead4321\n" > +" .text\n" > +" .globl main\n" > +" .type main, %function\n" > +"main:\n" > +" bl get_ptr\n" > +" str r0, [sp, #-8]!\n" > +" bl f\n" > +" str r0, [sp, #4]\n" > +" ldr r0, [sp]\n" > +" ldr r0, [r0]\n" > + CHECK (r1) > + CHECK (r2) > + CHECK (r3) > + CHECK (r4) > + CHECK (r5) > + CHECK (r6) > + CHECK (r7) > + CHECK (r8) > + CHECK (r9) > + CHECK (r10) > + CHECK (r11) > + CHECK (r12) > + CHECK (r13) > + CHECK (r14) > +" ldr r1, [sp, #4]\n" > + CHECK (r1) > +" mov r0, #0\n" > +" b exit\n" > +"1:\n" > +" b abort\n" > +" .size main, .-main" > +); > diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-2.c > b/gcc/testsuite/gcc.target/arm/stack-protector-2.c > new file mode 100644 > index 00000000000..266c36fdbc6 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/stack-protector-2.c > @@ -0,0 +1,6 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target fstack_protector } */ > +/* { dg-require-effective-target fpic } */ > +/* { dg-options "-fstack-protector-all -O2 -fpic" } */ > + > +#include "stack-protector-1.c"
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) ? Christophe > --- > gcc/config/arm/arm.md | 6 +- > gcc/config/arm/thumb1.md | 8 ++- > .../gcc.target/arm/stack-protector-1.c | 63 +++++++++++++++++++ > .../gcc.target/arm/stack-protector-2.c | 6 ++ > 4 files changed, 78 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-1.c > create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-2.c > > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md > index a6a31f8f4ef..dd13c77e889 100644 > --- a/gcc/config/arm/arm.md > +++ b/gcc/config/arm/arm.md > @@ -9320,6 +9320,8 @@ (define_insn_and_split "*stack_protect_combined_test_insn" > [(set_attr "arch" "t1,32")] > ) > > +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the > +;; canary value does not live beyond the end of this sequence. > (define_insn "arm_stack_protect_test_insn" > [(set (reg:CC_Z CC_REGNUM) > (compare:CC_Z (unspec:SI [(match_operand:SI 1 "memory_operand" "m,m") > @@ -9329,8 +9331,8 @@ (define_insn "arm_stack_protect_test_insn" > (clobber (match_operand:SI 0 "register_operand" "=&l,&r")) > (clobber (match_dup 2))] > "TARGET_32BIT" > - "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0" > - [(set_attr "length" "8,12") > + "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;mov\t%2, #0" > + [(set_attr "length" "12,16") > (set_attr "conds" "set") > (set_attr "type" "multiple") > (set_attr "arch" "t,32")] > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md > index 24861635fa5..0ff819090d9 100644 > --- a/gcc/config/arm/thumb1.md > +++ b/gcc/config/arm/thumb1.md > @@ -2020,6 +2020,8 @@ (define_insn_and_split "thumb_eh_return" > [(set_attr "type" "mov_reg")] > ) > > +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the > +;; canary value does not live beyond the end of this sequence. > (define_insn "thumb1_stack_protect_test_insn" > [(set (match_operand:SI 0 "register_operand" "=&l") > (unspec:SI [(match_operand:SI 1 "memory_operand" "m") > @@ -2027,9 +2029,9 @@ (define_insn "thumb1_stack_protect_test_insn" > UNSPEC_SP_TEST)) > (clobber (match_dup 2))] > "TARGET_THUMB1" > - "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0" > - [(set_attr "length" "8") > - (set_attr "conds" "set") > + "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;movs\t%2, #0" > + [(set_attr "length" "10") > + (set_attr "conds" "clob") > (set_attr "type" "multiple")] > ) > > diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-1.c b/gcc/testsuite/gcc.target/arm/stack-protector-1.c > new file mode 100644 > index 00000000000..b03ea14c4e2 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/stack-protector-1.c > @@ -0,0 +1,63 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target fstack_protector } */ > +/* { dg-options "-fstack-protector-all -O2" } */ > + > +extern volatile long *stack_chk_guard_ptr; > + > +volatile long * > +get_ptr (void) > +{ > + return stack_chk_guard_ptr; > +} > + > +void __attribute__ ((noipa)) > +f (void) > +{ > + volatile int x; > + x = 1; > + x += 1; > +} > + > +#define CHECK(REG) "\tcmp\tr0, " #REG "\n\tbeq\t1f\n" > + > +asm ( > +" .data\n" > +" .align 3\n" > +" .globl stack_chk_guard_ptr\n" > +"stack_chk_guard_ptr:\n" > +" .word __stack_chk_guard\n" > +" .weak __stack_chk_guard\n" > +"__stack_chk_guard:\n" > +" .word 0xdead4321\n" > +" .text\n" > +" .globl main\n" > +" .type main, %function\n" > +"main:\n" > +" bl get_ptr\n" > +" str r0, [sp, #-8]!\n" > +" bl f\n" > +" str r0, [sp, #4]\n" > +" ldr r0, [sp]\n" > +" ldr r0, [r0]\n" > + CHECK (r1) > + CHECK (r2) > + CHECK (r3) > + CHECK (r4) > + CHECK (r5) > + CHECK (r6) > + CHECK (r7) > + CHECK (r8) > + CHECK (r9) > + CHECK (r10) > + CHECK (r11) > + CHECK (r12) > + CHECK (r13) > + CHECK (r14) > +" ldr r1, [sp, #4]\n" > + CHECK (r1) > +" mov r0, #0\n" > +" b exit\n" > +"1:\n" > +" b abort\n" > +" .size main, .-main" > +); > diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-2.c b/gcc/testsuite/gcc.target/arm/stack-protector-2.c > new file mode 100644 > index 00000000000..266c36fdbc6 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/stack-protector-2.c > @@ -0,0 +1,6 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target fstack_protector } */ > +/* { dg-require-effective-target fpic } */ > +/* { dg-options "-fstack-protector-all -O2 -fpic" } */ > + > +#include "stack-protector-1.c"
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? Thanks, Richard
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 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)
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 :-) > 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. Richard
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
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index a6a31f8f4ef..dd13c77e889 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -9320,6 +9320,8 @@ (define_insn_and_split "*stack_protect_combined_test_insn" [(set_attr "arch" "t1,32")] ) +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the +;; canary value does not live beyond the end of this sequence. (define_insn "arm_stack_protect_test_insn" [(set (reg:CC_Z CC_REGNUM) (compare:CC_Z (unspec:SI [(match_operand:SI 1 "memory_operand" "m,m") @@ -9329,8 +9331,8 @@ (define_insn "arm_stack_protect_test_insn" (clobber (match_operand:SI 0 "register_operand" "=&l,&r")) (clobber (match_dup 2))] "TARGET_32BIT" - "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0" - [(set_attr "length" "8,12") + "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;mov\t%2, #0" + [(set_attr "length" "12,16") (set_attr "conds" "set") (set_attr "type" "multiple") (set_attr "arch" "t,32")] diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md index 24861635fa5..0ff819090d9 100644 --- a/gcc/config/arm/thumb1.md +++ b/gcc/config/arm/thumb1.md @@ -2020,6 +2020,8 @@ (define_insn_and_split "thumb_eh_return" [(set_attr "type" "mov_reg")] ) +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the +;; canary value does not live beyond the end of this sequence. (define_insn "thumb1_stack_protect_test_insn" [(set (match_operand:SI 0 "register_operand" "=&l") (unspec:SI [(match_operand:SI 1 "memory_operand" "m") @@ -2027,9 +2029,9 @@ (define_insn "thumb1_stack_protect_test_insn" UNSPEC_SP_TEST)) (clobber (match_dup 2))] "TARGET_THUMB1" - "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0" - [(set_attr "length" "8") - (set_attr "conds" "set") + "ldr\t%0, [%2]\;ldr\t%2, %1\;eors\t%0, %2, %0\;movs\t%2, #0" + [(set_attr "length" "10") + (set_attr "conds" "clob") (set_attr "type" "multiple")] ) diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-1.c b/gcc/testsuite/gcc.target/arm/stack-protector-1.c new file mode 100644 index 00000000000..b03ea14c4e2 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/stack-protector-1.c @@ -0,0 +1,63 @@ +/* { dg-do run } */ +/* { dg-require-effective-target fstack_protector } */ +/* { dg-options "-fstack-protector-all -O2" } */ + +extern volatile long *stack_chk_guard_ptr; + +volatile long * +get_ptr (void) +{ + return stack_chk_guard_ptr; +} + +void __attribute__ ((noipa)) +f (void) +{ + volatile int x; + x = 1; + x += 1; +} + +#define CHECK(REG) "\tcmp\tr0, " #REG "\n\tbeq\t1f\n" + +asm ( +" .data\n" +" .align 3\n" +" .globl stack_chk_guard_ptr\n" +"stack_chk_guard_ptr:\n" +" .word __stack_chk_guard\n" +" .weak __stack_chk_guard\n" +"__stack_chk_guard:\n" +" .word 0xdead4321\n" +" .text\n" +" .globl main\n" +" .type main, %function\n" +"main:\n" +" bl get_ptr\n" +" str r0, [sp, #-8]!\n" +" bl f\n" +" str r0, [sp, #4]\n" +" ldr r0, [sp]\n" +" ldr r0, [r0]\n" + CHECK (r1) + CHECK (r2) + CHECK (r3) + CHECK (r4) + CHECK (r5) + CHECK (r6) + CHECK (r7) + CHECK (r8) + CHECK (r9) + CHECK (r10) + CHECK (r11) + CHECK (r12) + CHECK (r13) + CHECK (r14) +" ldr r1, [sp, #4]\n" + CHECK (r1) +" mov r0, #0\n" +" b exit\n" +"1:\n" +" b abort\n" +" .size main, .-main" +); diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-2.c b/gcc/testsuite/gcc.target/arm/stack-protector-2.c new file mode 100644 index 00000000000..266c36fdbc6 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/stack-protector-2.c @@ -0,0 +1,6 @@ +/* { dg-do run } */ +/* { dg-require-effective-target fstack_protector } */ +/* { dg-require-effective-target fpic } */ +/* { dg-options "-fstack-protector-all -O2 -fpic" } */ + +#include "stack-protector-1.c"