diff mbox series

arm: Add a couple of extra stack-protector tests

Message ID mptr1qse41l.fsf@arm.com
State New
Headers show
Series arm: Add a couple of extra stack-protector tests | expand

Commit Message

Richard Sandiford Sept. 23, 2020, 6:33 p.m. UTC 
These tests were inspired by the corresponding aarch64 ones that I just
committed.  They already pass.

Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
OK for trunk?

Richard


gcc/testsuite/
	* gcc.target/arm/stack-protector-5.c: New test.
	* gcc.target/arm/stack-protector-6.c: Likewise.
---
 .../gcc.target/arm/stack-protector-5.c        | 21 +++++++++++++++++++
 .../gcc.target/arm/stack-protector-6.c        |  8 +++++++
 2 files changed, 29 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-5.c
 create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-6.c

Comments

Kyrylo Tkachov Sept. 24, 2020, 8:29 a.m. UTC | #1 
Hi Richard,

> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: 23 September 2020 19:34
> 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: Add a couple of extra stack-protector tests
> 
> These tests were inspired by the corresponding aarch64 ones that I just
> committed.  They already pass.
> 
> Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
> OK for trunk?

Ok. Do they also need to go on the branches when the fix is backported?
Thanks,
Kyrill

> 
> Richard
> 
> 
> gcc/testsuite/
> 	* gcc.target/arm/stack-protector-5.c: New test.
> 	* gcc.target/arm/stack-protector-6.c: Likewise.
> ---
>  .../gcc.target/arm/stack-protector-5.c        | 21 +++++++++++++++++++
>  .../gcc.target/arm/stack-protector-6.c        |  8 +++++++
>  2 files changed, 29 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-5.c
>  create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-6.c
> 
> diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-5.c
> b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
> new file mode 100644
> index 00000000000..b808b11aa3d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fstack-protector-all -O2" } */
> +
> +void __attribute__ ((noipa))
> +f (void)
> +{
> +  volatile int x;
> +  asm volatile ("" :::
> +		"r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> +		"r8", "r9", "r10", "r11", "r12", "r14");
> +}
> +
> +/* The register clobbers above should not generate any single LDRs or STRs;
> +   all registers should be pushed and popped using register lists.  The only
> +   STRs should therefore be those associated with the stack protector tests
> +   themselves.
> +
> +   Make sure the address of the canary is not spilled and reloaded,
> +   since that would give the attacker an opportunity to change the
> +   canary value.  */
> +/* { dg-final { scan-assembler-times {\tstr\t} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-6.c
> b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
> new file mode 100644
> index 00000000000..f8eec878bd6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-options "-fstack-protector-all -O2 -fpic" } */
> +
> +#include "stack-protector-5.c"
> +
> +/* See the comment in stack-protector-5.c.  */
> +/* { dg-final { scan-assembler-times {\tstr\t} 1 } } */
Richard Sandiford Sept. 24, 2020, 9 a.m. UTC | #2 
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:
> Hi Richard,
>
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: 23 September 2020 19:34
>> 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: Add a couple of extra stack-protector tests
>> 
>> These tests were inspired by the corresponding aarch64 ones that I just
>> committed.  They already pass.
>> 
>> Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
>> OK for trunk?
>
> Ok. Do they also need to go on the branches when the fix is backported?

There's not really an associated fix for this.  It's more just a defensive
patch: it's trying to make sure that the equivalent of the aarch64 bug
doesn't creep (back) into arm.  It was the same idea in the other direction
for 0f0b00033a71ff728d6fab6f9d: I've no evidence that those tests ever
failed on aarch64, but it seemed like a good idea to add aarch64
equivalents of the failing arm tests.

Thanks,
Richard
Christophe Lyon Sept. 28, 2020, 7:57 p.m. UTC | #3 
On Wed, 23 Sep 2020 at 20:33, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> These tests were inspired by the corresponding aarch64 ones that I just
> committed.  They already pass.
>
> Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
> OK for trunk?
>
> Richard
>
>
> gcc/testsuite/
>         * gcc.target/arm/stack-protector-5.c: New test.
>         * gcc.target/arm/stack-protector-6.c: Likewise.
> ---

Hi Richard,

These new tests fail when compiling for cortex-a15 and cortex-a57...
There are 2 "str" instructions generated, the code is much longer than
for cortex-a9 for instance.

They pass with cortex-a9, cortex-a5 and arm10tdmi.

Christophe

>  .../gcc.target/arm/stack-protector-5.c        | 21 +++++++++++++++++++
>  .../gcc.target/arm/stack-protector-6.c        |  8 +++++++
>  2 files changed, 29 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-5.c
>  create mode 100644 gcc/testsuite/gcc.target/arm/stack-protector-6.c
>
> diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-5.c b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
> new file mode 100644
> index 00000000000..b808b11aa3d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fstack-protector-all -O2" } */
> +
> +void __attribute__ ((noipa))
> +f (void)
> +{
> +  volatile int x;
> +  asm volatile ("" :::
> +               "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> +               "r8", "r9", "r10", "r11", "r12", "r14");
> +}
> +
> +/* The register clobbers above should not generate any single LDRs or STRs;
> +   all registers should be pushed and popped using register lists.  The only
> +   STRs should therefore be those associated with the stack protector tests
> +   themselves.
> +
> +   Make sure the address of the canary is not spilled and reloaded,
> +   since that would give the attacker an opportunity to change the
> +   canary value.  */
> +/* { dg-final { scan-assembler-times {\tstr\t} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-6.c b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
> new file mode 100644
> index 00000000000..f8eec878bd6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-options "-fstack-protector-all -O2 -fpic" } */
> +
> +#include "stack-protector-5.c"
> +
> +/* See the comment in stack-protector-5.c.  */
> +/* { dg-final { scan-assembler-times {\tstr\t} 1 } } */
Richard Sandiford Oct. 13, 2020, 1:51 p.m. UTC | #4 
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Wed, 23 Sep 2020 at 20:33, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> These tests were inspired by the corresponding aarch64 ones that I just
>> committed.  They already pass.
>>
>> Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
>> OK for trunk?
>>
>> Richard
>>
>>
>> gcc/testsuite/
>>         * gcc.target/arm/stack-protector-5.c: New test.
>>         * gcc.target/arm/stack-protector-6.c: Likewise.
>> ---
>
> Hi Richard,
>
> These new tests fail when compiling for cortex-a15 and cortex-a57...
> There are 2 "str" instructions generated, the code is much longer than
> for cortex-a9 for instance.
>
> They pass with cortex-a9, cortex-a5 and arm10tdmi.

Gah, thanks for the heads-up.  I've applied the below as obvious
after testing on arm-linux-gnueabihf and armeb-eabi.

Richard
From f694a0d2edc025cb54657cb804960f97a31fbda2 Mon Sep 17 00:00:00 2001
From: Richard Sandiford <richard.sandiford@arm.com>
Date: Tue, 13 Oct 2020 14:50:24 +0100
Subject: [PATCH] [arm] Use -Os for stack-protector-[56].c tests

Using -O2 made the tests subject to LDRD vs. LDM tuning.
The simplest fix seems to be to use -Os, so that LDM is
unequivocally a win.

gcc/testsuite/
	* gcc.target/arm/stack-protector-5.c: Use -Os rather than -O2.
	* gcc.target/arm/stack-protector-6.c: Likewise.
---
 gcc/testsuite/gcc.target/arm/stack-protector-5.c | 2 +-
 gcc/testsuite/gcc.target/arm/stack-protector-6.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-5.c b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
index b808b11aa3d..ae70b99efc4 100644
--- a/gcc/testsuite/gcc.target/arm/stack-protector-5.c
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fstack-protector-all -O2" } */
+/* { dg-options "-fstack-protector-all -Os" } */
 
 void __attribute__ ((noipa))
 f (void)
diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-6.c b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
index f8eec878bd6..2b7e6f72ea0 100644
--- a/gcc/testsuite/gcc.target/arm/stack-protector-6.c
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-require-effective-target fpic } */
-/* { dg-options "-fstack-protector-all -O2 -fpic" } */
+/* { dg-options "-fstack-protector-all -Os -fpic" } */
 
 #include "stack-protector-5.c"
Christophe Lyon Oct. 13, 2020, 2:01 p.m. UTC | #5 
On Tue, 13 Oct 2020 at 15:51, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> > On Wed, 23 Sep 2020 at 20:33, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> These tests were inspired by the corresponding aarch64 ones that I just
> >> committed.  They already pass.
> >>
> >> Tested on arm-linux-gnueabi, arm-linux-gnueabihf and armeb-eabi.
> >> OK for trunk?
> >>
> >> Richard
> >>
> >>
> >> gcc/testsuite/
> >>         * gcc.target/arm/stack-protector-5.c: New test.
> >>         * gcc.target/arm/stack-protector-6.c: Likewise.
> >> ---
> >
> > Hi Richard,
> >
> > These new tests fail when compiling for cortex-a15 and cortex-a57...
> > There are 2 "str" instructions generated, the code is much longer than
> > for cortex-a9 for instance.
> >
> > They pass with cortex-a9, cortex-a5 and arm10tdmi.
>
> Gah, thanks for the heads-up.  I've applied the below as obvious
> after testing on arm-linux-gnueabihf and armeb-eabi.
>

Nice, I hadn't thought of that workaround.

> Richard
>
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-5.c b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
new file mode 100644
index 00000000000..b808b11aa3d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-5.c
@@ -0,0 +1,21 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fstack-protector-all -O2" } */
+
+void __attribute__ ((noipa))
+f (void)
+{
+  volatile int x;
+  asm volatile ("" :::
+		"r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
+		"r8", "r9", "r10", "r11", "r12", "r14");
+}
+
+/* The register clobbers above should not generate any single LDRs or STRs;
+   all registers should be pushed and popped using register lists.  The only
+   STRs should therefore be those associated with the stack protector tests
+   themselves.
+
+   Make sure the address of the canary is not spilled and reloaded,
+   since that would give the attacker an opportunity to change the
+   canary value.  */
+/* { dg-final { scan-assembler-times {\tstr\t} 1 } } */
diff --git a/gcc/testsuite/gcc.target/arm/stack-protector-6.c b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
new file mode 100644
index 00000000000..f8eec878bd6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stack-protector-6.c
@@ -0,0 +1,8 @@ 
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-options "-fstack-protector-all -O2 -fpic" } */
+
+#include "stack-protector-5.c"
+
+/* See the comment in stack-protector-5.c.  */
+/* { dg-final { scan-assembler-times {\tstr\t} 1 } } */