diff mbox series

[2/4] aarch64: fix __builtin_eh_return with pac-ret [PR94891]

Message ID f3791bcede45ce3d7c55f0a5273c8ffc4aa0d7f8.1591374489.git.szabolcs.nagy@arm.com
State New
Headers show
Series aarch64: avoid exposing signed return addresses [PR94891] | expand

Commit Message

Szabolcs Nagy June 5, 2020, 4:51 p.m. UTC 
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.

2020-06-04  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* config/aarch64/aarch64.c (aarch64_return_address_signing_enabled):
	Disable return address signing if __builtin_eh_return is used.
---
 gcc/config/aarch64/aarch64.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Szabolcs Nagy June 26, 2020, 2:48 p.m. UTC | #1 
The 06/05/2020 17:51, Szabolcs Nagy wrote:
> 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.
> 
> 2020-06-04  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	* config/aarch64/aarch64.c (aarch64_return_address_signing_enabled):
> 	Disable return address signing if __builtin_eh_return is used.

ping.

this fixes a correctness bug in pac-ret, tested
on aarch64, with only the following regressions:

FAIL: gcc.target/aarch64/return_address_sign_1.c scan-assembler-times autiasp 4
FAIL: gcc.target/aarch64/return_address_sign_1.c scan-assembler-times paciasp 4
FAIL: gcc.target/aarch64/return_address_sign_b_1.c scan-assembler-times autibsp 4
FAIL: gcc.target/aarch64/return_address_sign_b_1.c scan-assembler-times pacibsp 4

which can be fixed by

-/* { 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 } } */

since __builtin_eh_return path no longer uses pac/aut.

> ---
>  gcc/config/aarch64/aarch64.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 6a2f85c4af7..d9557f7c0a2 100644
> --- 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.  */
> +  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
> -- 
> 2.17.1
> 

--
Kyrylo Tkachov July 8, 2020, 1:24 p.m. UTC | #2 
Hi Szabolcs,

> -----Original Message-----
> From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of
> Szabolcs Nagy
> Sent: 26 June 2020 15:49
> To: gcc-patches@gcc.gnu.org
> Cc: fweimer@redhat.com; Richard Earnshaw <Richard.Earnshaw@arm.com>;
> Daniel Kiss <Daniel.Kiss@arm.com>
> Subject: Re: [PATCH 2/4] aarch64: fix __builtin_eh_return with pac-ret
> [PR94891]
> 
> The 06/05/2020 17:51, Szabolcs Nagy wrote:
> > 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.
> >
> > 2020-06-04  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> >
> > 	* config/aarch64/aarch64.c
> (aarch64_return_address_signing_enabled):
> > 	Disable return address signing if __builtin_eh_return is used.
> 
> ping.
> 
> this fixes a correctness bug in pac-ret, tested
> on aarch64, with only the following regressions:
> 
> FAIL: gcc.target/aarch64/return_address_sign_1.c scan-assembler-times
> autiasp 4
> FAIL: gcc.target/aarch64/return_address_sign_1.c scan-assembler-times
> paciasp 4
> FAIL: gcc.target/aarch64/return_address_sign_b_1.c scan-assembler-times
> autibsp 4
> FAIL: gcc.target/aarch64/return_address_sign_b_1.c scan-assembler-times
> pacibsp 4
> 
> which can be fixed by
> 
> -/* { 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 } } */
> 
> since __builtin_eh_return path no longer uses pac/aut.

This is ok but...

> 
> > ---
> >  gcc/config/aarch64/aarch64.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index 6a2f85c4af7..d9557f7c0a2 100644
> > --- 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.

Thanks,
Kyrill

> > +  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
> > --
> > 2.17.1
> >
> 
> --
Szabolcs Nagy July 8, 2020, 3:47 p.m. UTC | #3 
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.
Kyrylo Tkachov July 8, 2020, 3:54 p.m. UTC | #4 
> -----Original Message-----
> From: Szabolcs Nagy <Szabolcs.Nagy@arm.com>
> Sent: 08 July 2020 16:48
> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Cc: gcc-patches@gcc.gnu.org; fweimer@redhat.com; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; Daniel Kiss <Daniel.Kiss@arm.com>
> Subject: Re: [PATCH 2/4] aarch64: fix __builtin_eh_return with pac-ret
> [PR94891]
> 
> 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.

Nice, thanks.
Kyrill
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 6a2f85c4af7..d9557f7c0a2 100644
--- 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.  */
+  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