* [PATCH] arm: Add a couple of extra stack-protector tests
@ 2020-09-23 18:33 Richard Sandiford
2020-09-24 8:29 ` Kyrylo Tkachov
2020-09-28 19:57 ` Christophe Lyon
0 siblings, 2 replies; 6+ messages in thread
From: Richard Sandiford @ 2020-09-23 18:33 UTC (permalink / raw)
To: gcc-patches; +Cc: nickc, richard.earnshaw, ramana.radhakrishnan, kyrylo.tkachov
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
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 } } */
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] arm: Add a couple of extra stack-protector tests
2020-09-23 18:33 [PATCH] arm: Add a couple of extra stack-protector tests Richard Sandiford
@ 2020-09-24 8:29 ` Kyrylo Tkachov
2020-09-24 9:00 ` Richard Sandiford
2020-09-28 19:57 ` Christophe Lyon
1 sibling, 1 reply; 6+ messages in thread
From: Kyrylo Tkachov @ 2020-09-24 8:29 UTC (permalink / raw)
To: Richard Sandiford, gcc-patches
Cc: nickc, Richard Earnshaw, Ramana Radhakrishnan
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 } } */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] arm: Add a couple of extra stack-protector tests
2020-09-24 8:29 ` Kyrylo Tkachov
@ 2020-09-24 9:00 ` Richard Sandiford
0 siblings, 0 replies; 6+ messages in thread
From: Richard Sandiford @ 2020-09-24 9:00 UTC (permalink / raw)
To: Kyrylo Tkachov; +Cc: gcc-patches, nickc, Richard Earnshaw, Ramana Radhakrishnan
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm: Add a couple of extra stack-protector tests
2020-09-23 18:33 [PATCH] arm: Add a couple of extra stack-protector tests Richard Sandiford
2020-09-24 8:29 ` Kyrylo Tkachov
@ 2020-09-28 19:57 ` Christophe Lyon
2020-10-13 13:51 ` Richard Sandiford
1 sibling, 1 reply; 6+ messages in thread
From: Christophe Lyon @ 2020-09-28 19:57 UTC (permalink / raw)
To: gcc Patches, nick clifton, Richard Earnshaw,
Ramana Radhakrishnan, Kyrylo Tkachov, Richard Sandiford
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 } } */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] arm: Add a couple of extra stack-protector tests
2020-09-28 19:57 ` Christophe Lyon
@ 2020-10-13 13:51 ` Richard Sandiford
2020-10-13 14:01 ` Christophe Lyon
0 siblings, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2020-10-13 13:51 UTC (permalink / raw)
To: Christophe Lyon
Cc: gcc Patches, nick clifton, Richard Earnshaw,
Ramana Radhakrishnan, Kyrylo Tkachov
[-- Attachment #1: Type: text/plain, Size: 879 bytes --]
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
[-- Attachment #2: 0001-arm-Use-Os-for-stack-protector-56-.c-tests.patch --]
[-- Type: text/plain, Size: 1608 bytes --]
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"
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] arm: Add a couple of extra stack-protector tests
2020-10-13 13:51 ` Richard Sandiford
@ 2020-10-13 14:01 ` Christophe Lyon
0 siblings, 0 replies; 6+ messages in thread
From: Christophe Lyon @ 2020-10-13 14:01 UTC (permalink / raw)
To: Christophe Lyon, gcc Patches, nick clifton, Richard Earnshaw,
Ramana Radhakrishnan, Kyrylo Tkachov, Richard Sandiford
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-13 14:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 18:33 [PATCH] arm: Add a couple of extra stack-protector tests Richard Sandiford
2020-09-24 8:29 ` Kyrylo Tkachov
2020-09-24 9:00 ` Richard Sandiford
2020-09-28 19:57 ` Christophe Lyon
2020-10-13 13:51 ` Richard Sandiford
2020-10-13 14:01 ` Christophe Lyon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).