The 07/08/2020 13:24, Kyrylo Tkachov wrote:
> Hi Szabolcs,
> > The 06/05/2020 17:51, Szabolcs Nagy wrote:
> > > --- a/gcc/config/aarch64/aarch64.c
> > > +++ b/gcc/config/aarch64/aarch64.c
> > > @@ -6954,6 +6954,10 @@ aarch64_return_address_signing_enabled (void)
> > > /* This function should only be called after frame laid out. */
> > > gcc_assert (cfun->machine->frame.laid_out);
> > >
> > > + /* TODO: Big hammer handling of __builtin_eh_return. */
>
> ... I don't think this comment is very useful. Please make it a bit more
> descriptive. If you want to leave the TODO here, please give a more concrete
> action plan.
see attached patch with more detailed comment and commit message.
>From e0f4b9b94be1b59c3141abc136ea387bb43fcdce Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <szabolcs.n...@arm.com>
Date: Thu, 4 Jun 2020 13:42:16 +0100
Subject: [PATCH v2] aarch64: fix __builtin_eh_return with pac-ret [PR94891]
The handler argument must not be signed since that may come from
outside the current module and exposing signed addresses is a pointer
ABI break. (The signed address also may not be representable as void *
which is why pac-ret is currently broken on ilp32.)
There is no point protecting the eh return path with pointer auth
since arbitrary target can be reached with the instruction sequence
in the caller function anyway, however this is a big hammer solution
that turns off pac-ret for the caller completely not just on the eh
return path. A proper fix would change eh return to use an indirect
branch instead of ret (and ensure BTI j landing pads are in place),
this is not attempted so the patch remains small and backportable.
2020-07-08 Szabolcs Nagy <szabolcs.n...@arm.com>
gcc/ChangeLog:
PR target/94891
* config/aarch64/aarch64.c (aarch64_return_address_signing_enabled):
Disable return address signing if __builtin_eh_return is used.
gcc/testsuite/ChangeLog:
PR target/94891
* gcc.target/aarch64/return_address_sign_1.c: Update test.
* gcc.target/aarch64/return_address_sign_b_1.c: Likewise.
---
gcc/config/aarch64/aarch64.c | 11 +++++++++++
.../gcc.target/aarch64/return_address_sign_1.c | 8 ++++----
.../gcc.target/aarch64/return_address_sign_b_1.c | 8 ++++----
3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5865d1d7b78..3ad96e07b7b 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -6957,6 +6957,17 @@ aarch64_return_address_signing_enabled (void)
/* This function should only be called after frame laid out. */
gcc_assert (cfun->machine->frame.laid_out);
+ /* Turn return address signing off in any function that uses
+ __builtin_eh_return. The address passed to __builtin_eh_return
+ is not signed so either it has to be signed (with original sp)
+ or the code path that uses it has to avoid authenticating it.
+ Currently eh return introduces a return to anywhere gadget, no
+ matter what we do here since it uses ret with user provided
+ address. An ideal fix for that is to use indirect branch which
+ can be protected with BTI j (to some extent). */
+ if (crtl->calls_eh_return)
+ return false;
+
/* If signing scope is AARCH64_FUNCTION_NON_LEAF, we only sign a leaf function
if its LR is pushed onto stack. */
return (aarch64_ra_sign_scope == AARCH64_FUNCTION_ALL
diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
index 0140bee194f..232ba67ade0 100644
--- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
@@ -41,12 +41,12 @@ func3 (int a, int b, int c)
void __attribute__ ((target ("arch=armv8.3-a")))
func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
{
- /* paciasp */
+ /* no paciasp */
*ptr = imm1 + foo (imm1) + imm2;
__builtin_eh_return (offset, handler);
- /* autiasp */
+ /* no autiasp */
return;
}
-/* { dg-final { scan-assembler-times "autiasp" 4 } } */
-/* { dg-final { scan-assembler-times "paciasp" 4 } } */
+/* { dg-final { scan-assembler-times "autiasp" 3 } } */
+/* { dg-final { scan-assembler-times "paciasp" 3 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
index 32d788ddf3f..43e32ab6cb7 100644
--- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
@@ -41,12 +41,12 @@ func3 (int a, int b, int c)
void __attribute__ ((target ("arch=armv8.3-a")))
func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
{
- /* pacibsp */
+ /* no pacibsp */
*ptr = imm1 + foo (imm1) + imm2;
__builtin_eh_return (offset, handler);
- /* autibsp */
+ /* no autibsp */
return;
}
-/* { dg-final { scan-assembler-times "pacibsp" 4 } } */
-/* { dg-final { scan-assembler-times "autibsp" 4 } } */
+/* { dg-final { scan-assembler-times "pacibsp" 3 } } */
+/* { dg-final { scan-assembler-times "autibsp" 3 } } */
--
2.17.1
