diff mbox series

[3/4] libgcc: fix the handling of return address mangling [PR94891]

Message ID 90c191bdfe59f5c42a6d1c35733528d71ef688b9.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 
Mangling, currently only used on AArch64 for return address signing,
is an internal representation that should not be exposed via

  __builtin_return_address return value,
  __builtin_eh_return handler argument,
  _Unwind_DebugHook handler argument.

Note that a mangled address might not even fit into a void *, e.g.
with AArch64 ilp32 ABI the return address is stored as 64bit, so
the mangled return address cannot be accessed via _Unwind_GetPtr.

This patch changes the unwinder hooks as follows:

MD_POST_EXTRACT_ROOT_ADDR is removed: root address comes from
__builtin_return_address which is not mangled.

MD_POST_EXTRACT_FRAME_ADDR is renamed to MD_DEMANGLE_RETURN_ADDR,
it now operates on _Unwind_Word instead of void *, so the hook
should work when return address signing is enabled on AArch64 ilp32.
(But for that __builtin_aarch64_autia1716 should be fixed to operate
on 64bit input instead of a void *.)

MD_POST_FROB_EH_HANDLER_ADDR is removed: it is the responsibility of
__builtin_eh_return to do the mangling if necessary.

libgcc/ChangeLog:

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

	* config/aarch64/aarch64-unwind.h (MD_POST_EXTRACT_ROOT_ADDR): Remove.
	(MD_POST_FROB_EH_HANDLER_ADDR): Remove.
	(MD_POST_EXTRACT_FRAME_ADDR): Rename to ...
	(MD_DEMANGLE_RETURN_ADDR): This.
	(aarch64_post_extract_frame_addr): Rename to ...
	(aarch64_demangle_return_addr): This.
	(aarch64_post_frob_eh_handler_addr): Remove.
	* unwind-dw2.c (uw_update_context): Demangle return address.
	(uw_frob_return_addr): Remove.
---
 libgcc/config/aarch64/aarch64-unwind.h | 34 ++++----------------------
 libgcc/unwind-dw2.c                    | 34 ++++++--------------------
 2 files changed, 13 insertions(+), 55 deletions(-)

Comments

Luis Machado June 8, 2020, 12:12 p.m. UTC | #1 
Hi Szabolcs,

Just to confirm, this is a "unwinder debugger hook ABI" change only in 
the sense that the generated DWARF will be changed, right? So no further 
action from DWARF consumers will be needed. Is that understanding correct?

On 6/5/20 1:51 PM, Szabolcs Nagy wrote:
> Mangling, currently only used on AArch64 for return address signing,
> is an internal representation that should not be exposed via
> 
>    __builtin_return_address return value,
>    __builtin_eh_return handler argument,
>    _Unwind_DebugHook handler argument.
> 
> Note that a mangled address might not even fit into a void *, e.g.
> with AArch64 ilp32 ABI the return address is stored as 64bit, so
> the mangled return address cannot be accessed via _Unwind_GetPtr.
> 
> This patch changes the unwinder hooks as follows:
> 
> MD_POST_EXTRACT_ROOT_ADDR is removed: root address comes from
> __builtin_return_address which is not mangled.
> 
> MD_POST_EXTRACT_FRAME_ADDR is renamed to MD_DEMANGLE_RETURN_ADDR,
> it now operates on _Unwind_Word instead of void *, so the hook
> should work when return address signing is enabled on AArch64 ilp32.
> (But for that __builtin_aarch64_autia1716 should be fixed to operate
> on 64bit input instead of a void *.)
> 
> MD_POST_FROB_EH_HANDLER_ADDR is removed: it is the responsibility of
> __builtin_eh_return to do the mangling if necessary.
> 
> libgcc/ChangeLog:
> 
> 2020-06-04  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	* config/aarch64/aarch64-unwind.h (MD_POST_EXTRACT_ROOT_ADDR): Remove.
> 	(MD_POST_FROB_EH_HANDLER_ADDR): Remove.
> 	(MD_POST_EXTRACT_FRAME_ADDR): Rename to ...
> 	(MD_DEMANGLE_RETURN_ADDR): This.
> 	(aarch64_post_extract_frame_addr): Rename to ...
> 	(aarch64_demangle_return_addr): This.
> 	(aarch64_post_frob_eh_handler_addr): Remove.
> 	* unwind-dw2.c (uw_update_context): Demangle return address.
> 	(uw_frob_return_addr): Remove.
> ---
>   libgcc/config/aarch64/aarch64-unwind.h | 34 ++++----------------------
>   libgcc/unwind-dw2.c                    | 34 ++++++--------------------
>   2 files changed, 13 insertions(+), 55 deletions(-)
> 
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index ed84a96db41..b1d732e0b2d 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -27,11 +27,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>   
>   #define DWARF_REGNUM_AARCH64_RA_STATE 34
>   
> -#define MD_POST_EXTRACT_ROOT_ADDR(addr)  __builtin_aarch64_xpaclri (addr)
> -#define MD_POST_EXTRACT_FRAME_ADDR(context, fs, addr) \
> -  aarch64_post_extract_frame_addr (context, fs, addr)
> -#define MD_POST_FROB_EH_HANDLER_ADDR(current, target, addr) \
> -  aarch64_post_frob_eh_handler_addr (current, target, addr)
> +#define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
> +  aarch64_demangle_return_addr (context, fs, addr)
>   #define MD_FROB_UPDATE_CONTEXT(context, fs) \
>     aarch64_frob_update_context (context, fs)
>   
> @@ -57,9 +54,10 @@ aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
>      using CFA of current frame.  */
>   
>   static inline void *
> -aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
> -				 _Unwind_FrameState *fs, void *addr)
> +aarch64_demangle_return_addr (struct _Unwind_Context *context,
> +			      _Unwind_FrameState *fs, _Unwind_Word addr_word)
>   {
> +  void *addr = (void *)addr_word;
>     if (context->flags & RA_SIGNED_BIT)
>       {
>         _Unwind_Word salt = (_Unwind_Word) context->cfa;
> @@ -71,28 +69,6 @@ aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
>       return addr;
>   }
>   
> -/* Do AArch64 private frob on exception handler's address HANDLER_ADDR before
> -   installing it into current context CURRENT.  TARGET is currently not used.
> -   We need to sign exception handler's address if CURRENT itself is signed.  */
> -
> -static inline void *
> -aarch64_post_frob_eh_handler_addr (struct _Unwind_Context *current,
> -				   struct _Unwind_Context *target
> -				   ATTRIBUTE_UNUSED,
> -				   void *handler_addr)
> -{
> -  if (current->flags & RA_SIGNED_BIT)
> -    {
> -      if (aarch64_cie_signed_with_b_key (current))
> -	return __builtin_aarch64_pacib1716 (handler_addr,
> -					    (_Unwind_Word) current->cfa);
> -      return __builtin_aarch64_pacia1716 (handler_addr,
> -					(_Unwind_Word) current->cfa);
> -    }
> -  else
> -    return handler_addr;
> -}
> -
>   /* Do AArch64 private initialization on CONTEXT based on frame info FS.  Mark
>      CONTEXT as return address signed if bit 0 of DWARF_REGNUM_AARCH64_RA_STATE is
>      set.  */
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 62d4a3d29a2..fe896565d2e 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -1538,11 +1538,14 @@ uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>       {
>         /* Compute the return address now, since the return address column
>   	 can change from frame to frame.  */
> -      context->ra = __builtin_extract_return_addr
> -	(_Unwind_GetPtr (context, fs->retaddr_column));
> -#ifdef MD_POST_EXTRACT_FRAME_ADDR
> -      context->ra = MD_POST_EXTRACT_FRAME_ADDR (context, fs, context->ra);
> +      void *ret_addr;
> +#ifdef MD_DEMANGLE_RETURN_ADDR
> +      _Unwind_Word ra = _Unwind_GetGR (context, fs->retaddr_column);
> +      ret_addr = MD_DEMANGLE_RETURN_ADDR (context, fs, ra);
> +#else
> +      ret_addr = _Unwind_GetPtr (context, fs->retaddr_column);
>   #endif
> +      context->ra = __builtin_extract_return_addr (ret_addr);
>       }
>   }
>   
> @@ -1577,9 +1580,6 @@ uw_init_context_1 (struct _Unwind_Context *context,
>   		   void *outer_cfa, void *outer_ra)
>   {
>     void *ra = __builtin_extract_return_addr (__builtin_return_address (0));
> -#ifdef MD_POST_EXTRACT_ROOT_ADDR
> -  ra = MD_POST_EXTRACT_ROOT_ADDR (ra);
> -#endif
>     _Unwind_FrameState fs;
>     _Unwind_SpTmp sp_slot;
>     _Unwind_Reason_Code code;
> @@ -1616,9 +1616,6 @@ uw_init_context_1 (struct _Unwind_Context *context,
>        initialization context, then we can't see it in the given
>        call frame data.  So have the initialization context tell us.  */
>     context->ra = __builtin_extract_return_addr (outer_ra);
> -#ifdef MD_POST_EXTRACT_ROOT_ADDR
> -  context->ra = MD_POST_EXTRACT_ROOT_ADDR (context->ra);
> -#endif
>   }
>   
>   static void _Unwind_DebugHook (void *, void *)
> @@ -1641,21 +1638,6 @@ _Unwind_DebugHook (void *cfa __attribute__ ((__unused__)),
>   #endif
>   }
>   
> -/* Frob exception handler's address kept in TARGET before installing into
> -   CURRENT context.  */
> -
> -static inline void *
> -uw_frob_return_addr (struct _Unwind_Context *current
> -		     __attribute__ ((__unused__)),
> -		     struct _Unwind_Context *target)
> -{
> -  void *ret_addr = __builtin_frob_return_addr (target->ra);
> -#ifdef MD_POST_FROB_EH_HANDLER_ADDR
> -  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
> -#endif
> -  return ret_addr;
> -}
> -
>   /* Install TARGET into CURRENT so that we can return to it.  This is a
>      macro because __builtin_eh_return must be invoked in the context of
>      our caller.  FRAMES is a number of frames to be unwind.
> @@ -1667,7 +1649,7 @@ uw_frob_return_addr (struct _Unwind_Context *current
>     do									\
>       {									\
>         long offset = uw_install_context_1 ((CURRENT), (TARGET));		\
> -      void *handler = uw_frob_return_addr ((CURRENT), (TARGET));	\
> +      void *handler = __builtin_frob_return_addr ((TARGET)->ra);	\
>         _Unwind_DebugHook ((TARGET)->cfa, handler);			\
>         _Unwind_Frames_Extra (FRAMES);					\
>         __builtin_eh_return (offset, handler);				\
>
Szabolcs Nagy June 8, 2020, 12:32 p.m. UTC | #2 
The 06/08/2020 09:12, Luis Machado wrote:
> Hi Szabolcs,
> 
> Just to confirm, this is a "unwinder debugger hook ABI" change only in the
> sense that the generated DWARF will be changed, right? So no further action
> from DWARF consumers will be needed. Is that understanding correct?

the _Unwind_DebugHook can currently be called
with a handler argument that's signed (see below),
i don't know how gdb uses this api (e.g. if it
uses DWARF info to check if the argument is signed),
but i plan to change all apis not to pass signed
pointers around since they can cause ABI issues.
(the debugger is special in that it has to know about
pointer auth anyway so it's not critical to change
this api, but i think it's better to do for consistency)

> On 6/5/20 1:51 PM, Szabolcs Nagy wrote:
> > Mangling, currently only used on AArch64 for return address signing,
> > is an internal representation that should not be exposed via
> > 
> >    __builtin_return_address return value,
> >    __builtin_eh_return handler argument,
> >    _Unwind_DebugHook handler argument.
...
> > -static inline void *
> > -uw_frob_return_addr (struct _Unwind_Context *current
> > -		     __attribute__ ((__unused__)),
> > -		     struct _Unwind_Context *target)
> > -{
> > -  void *ret_addr = __builtin_frob_return_addr (target->ra);
> > -#ifdef MD_POST_FROB_EH_HANDLER_ADDR
> > -  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
> > -#endif
> > -  return ret_addr;
> > -}
> > -
> >   /* Install TARGET into CURRENT so that we can return to it.  This is a
> >      macro because __builtin_eh_return must be invoked in the context of
> >      our caller.  FRAMES is a number of frames to be unwind.
> > @@ -1667,7 +1649,7 @@ uw_frob_return_addr (struct _Unwind_Context *current
> >     do									\
> >       {									\
> >         long offset = uw_install_context_1 ((CURRENT), (TARGET));		\
> > -      void *handler = uw_frob_return_addr ((CURRENT), (TARGET));	\
> > +      void *handler = __builtin_frob_return_addr ((TARGET)->ra);	\
> >         _Unwind_DebugHook ((TARGET)->cfa, handler);			\
> >         _Unwind_Frames_Extra (FRAMES);					\
> >         __builtin_eh_return (offset, handler);				\

handler is no longer signed after my patch.
(if signing is necessary then __builtin_eh_return
should deal with that.)
Szabolcs Nagy June 26, 2020, 2:51 p.m. UTC | #3 
The 06/05/2020 17:51, Szabolcs Nagy wrote:
> Mangling, currently only used on AArch64 for return address signing,
> is an internal representation that should not be exposed via
> 
>   __builtin_return_address return value,
>   __builtin_eh_return handler argument,
>   _Unwind_DebugHook handler argument.
> 
> Note that a mangled address might not even fit into a void *, e.g.
> with AArch64 ilp32 ABI the return address is stored as 64bit, so
> the mangled return address cannot be accessed via _Unwind_GetPtr.
> 
> This patch changes the unwinder hooks as follows:
> 
> MD_POST_EXTRACT_ROOT_ADDR is removed: root address comes from
> __builtin_return_address which is not mangled.
> 
> MD_POST_EXTRACT_FRAME_ADDR is renamed to MD_DEMANGLE_RETURN_ADDR,
> it now operates on _Unwind_Word instead of void *, so the hook
> should work when return address signing is enabled on AArch64 ilp32.
> (But for that __builtin_aarch64_autia1716 should be fixed to operate
> on 64bit input instead of a void *.)
> 
> MD_POST_FROB_EH_HANDLER_ADDR is removed: it is the responsibility of
> __builtin_eh_return to do the mangling if necessary.
> 
> libgcc/ChangeLog:
> 
> 2020-06-04  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	* config/aarch64/aarch64-unwind.h (MD_POST_EXTRACT_ROOT_ADDR): Remove.
> 	(MD_POST_FROB_EH_HANDLER_ADDR): Remove.
> 	(MD_POST_EXTRACT_FRAME_ADDR): Rename to ...
> 	(MD_DEMANGLE_RETURN_ADDR): This.
> 	(aarch64_post_extract_frame_addr): Rename to ...
> 	(aarch64_demangle_return_addr): This.
> 	(aarch64_post_frob_eh_handler_addr): Remove.
> 	* unwind-dw2.c (uw_update_context): Demangle return address.
> 	(uw_frob_return_addr): Remove.

ping. (adding Ian on cc)

tested without regressions on aarch64 with pac-ret.

> ---
>  libgcc/config/aarch64/aarch64-unwind.h | 34 ++++----------------------
>  libgcc/unwind-dw2.c                    | 34 ++++++--------------------
>  2 files changed, 13 insertions(+), 55 deletions(-)
> 
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index ed84a96db41..b1d732e0b2d 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -27,11 +27,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  
>  #define DWARF_REGNUM_AARCH64_RA_STATE 34
>  
> -#define MD_POST_EXTRACT_ROOT_ADDR(addr)  __builtin_aarch64_xpaclri (addr)
> -#define MD_POST_EXTRACT_FRAME_ADDR(context, fs, addr) \
> -  aarch64_post_extract_frame_addr (context, fs, addr)
> -#define MD_POST_FROB_EH_HANDLER_ADDR(current, target, addr) \
> -  aarch64_post_frob_eh_handler_addr (current, target, addr)
> +#define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
> +  aarch64_demangle_return_addr (context, fs, addr)
>  #define MD_FROB_UPDATE_CONTEXT(context, fs) \
>    aarch64_frob_update_context (context, fs)
>  
> @@ -57,9 +54,10 @@ aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
>     using CFA of current frame.  */
>  
>  static inline void *
> -aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
> -				 _Unwind_FrameState *fs, void *addr)
> +aarch64_demangle_return_addr (struct _Unwind_Context *context,
> +			      _Unwind_FrameState *fs, _Unwind_Word addr_word)
>  {
> +  void *addr = (void *)addr_word;
>    if (context->flags & RA_SIGNED_BIT)
>      {
>        _Unwind_Word salt = (_Unwind_Word) context->cfa;
> @@ -71,28 +69,6 @@ aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
>      return addr;
>  }
>  
> -/* Do AArch64 private frob on exception handler's address HANDLER_ADDR before
> -   installing it into current context CURRENT.  TARGET is currently not used.
> -   We need to sign exception handler's address if CURRENT itself is signed.  */
> -
> -static inline void *
> -aarch64_post_frob_eh_handler_addr (struct _Unwind_Context *current,
> -				   struct _Unwind_Context *target
> -				   ATTRIBUTE_UNUSED,
> -				   void *handler_addr)
> -{
> -  if (current->flags & RA_SIGNED_BIT)
> -    {
> -      if (aarch64_cie_signed_with_b_key (current))
> -	return __builtin_aarch64_pacib1716 (handler_addr,
> -					    (_Unwind_Word) current->cfa);
> -      return __builtin_aarch64_pacia1716 (handler_addr,
> -					(_Unwind_Word) current->cfa);
> -    }
> -  else
> -    return handler_addr;
> -}
> -
>  /* Do AArch64 private initialization on CONTEXT based on frame info FS.  Mark
>     CONTEXT as return address signed if bit 0 of DWARF_REGNUM_AARCH64_RA_STATE is
>     set.  */
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 62d4a3d29a2..fe896565d2e 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -1538,11 +1538,14 @@ uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>      {
>        /* Compute the return address now, since the return address column
>  	 can change from frame to frame.  */
> -      context->ra = __builtin_extract_return_addr
> -	(_Unwind_GetPtr (context, fs->retaddr_column));
> -#ifdef MD_POST_EXTRACT_FRAME_ADDR
> -      context->ra = MD_POST_EXTRACT_FRAME_ADDR (context, fs, context->ra);
> +      void *ret_addr;
> +#ifdef MD_DEMANGLE_RETURN_ADDR
> +      _Unwind_Word ra = _Unwind_GetGR (context, fs->retaddr_column);
> +      ret_addr = MD_DEMANGLE_RETURN_ADDR (context, fs, ra);
> +#else
> +      ret_addr = _Unwind_GetPtr (context, fs->retaddr_column);
>  #endif
> +      context->ra = __builtin_extract_return_addr (ret_addr);
>      }
>  }
>  
> @@ -1577,9 +1580,6 @@ uw_init_context_1 (struct _Unwind_Context *context,
>  		   void *outer_cfa, void *outer_ra)
>  {
>    void *ra = __builtin_extract_return_addr (__builtin_return_address (0));
> -#ifdef MD_POST_EXTRACT_ROOT_ADDR
> -  ra = MD_POST_EXTRACT_ROOT_ADDR (ra);
> -#endif
>    _Unwind_FrameState fs;
>    _Unwind_SpTmp sp_slot;
>    _Unwind_Reason_Code code;
> @@ -1616,9 +1616,6 @@ uw_init_context_1 (struct _Unwind_Context *context,
>       initialization context, then we can't see it in the given
>       call frame data.  So have the initialization context tell us.  */
>    context->ra = __builtin_extract_return_addr (outer_ra);
> -#ifdef MD_POST_EXTRACT_ROOT_ADDR
> -  context->ra = MD_POST_EXTRACT_ROOT_ADDR (context->ra);
> -#endif
>  }
>  
>  static void _Unwind_DebugHook (void *, void *)
> @@ -1641,21 +1638,6 @@ _Unwind_DebugHook (void *cfa __attribute__ ((__unused__)),
>  #endif
>  }
>  
> -/* Frob exception handler's address kept in TARGET before installing into
> -   CURRENT context.  */
> -
> -static inline void *
> -uw_frob_return_addr (struct _Unwind_Context *current
> -		     __attribute__ ((__unused__)),
> -		     struct _Unwind_Context *target)
> -{
> -  void *ret_addr = __builtin_frob_return_addr (target->ra);
> -#ifdef MD_POST_FROB_EH_HANDLER_ADDR
> -  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
> -#endif
> -  return ret_addr;
> -}
> -
>  /* Install TARGET into CURRENT so that we can return to it.  This is a
>     macro because __builtin_eh_return must be invoked in the context of
>     our caller.  FRAMES is a number of frames to be unwind.
> @@ -1667,7 +1649,7 @@ uw_frob_return_addr (struct _Unwind_Context *current
>    do									\
>      {									\
>        long offset = uw_install_context_1 ((CURRENT), (TARGET));		\
> -      void *handler = uw_frob_return_addr ((CURRENT), (TARGET));	\
> +      void *handler = __builtin_frob_return_addr ((TARGET)->ra);	\
>        _Unwind_DebugHook ((TARGET)->cfa, handler);			\
>        _Unwind_Frames_Extra (FRAMES);					\
>        __builtin_eh_return (offset, handler);				\
> -- 
> 2.17.1
> 

--
Richard Sandiford July 13, 2020, 11:18 a.m. UTC | #4 
Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> Mangling, currently only used on AArch64 for return address signing,
> is an internal representation that should not be exposed via
>
>   __builtin_return_address return value,
>   __builtin_eh_return handler argument,
>   _Unwind_DebugHook handler argument.
>
> Note that a mangled address might not even fit into a void *, e.g.
> with AArch64 ilp32 ABI the return address is stored as 64bit, so
> the mangled return address cannot be accessed via _Unwind_GetPtr.

Summarising what we talked about off-list wrt __builtin_eh_return
(in the context of this patch and 2/4):

* The unwinder routine is still a potential return-anywhere gadget with
  the current PAC-RET approach, since the caller of __builtin_eh_return
  has to sign a general address.  At best, the need to include the
  signing instruction in the gadget makes life harder for an attacker,
  if the signing instruction happens to be scheduled early.

* libgcc only provides an option to use PAC-RET + BTI together.

* With BTI, using a jump instead of a return gives better protection
  because it requires the EH handler address to be a valid jump target.
  The current PAC-RET approach is incompatible with doing that.

* BTI would make it harder for the jump itself to be used as a gadget.

* When BTI is enabled, it is safe to assume that all EH handlers already
  have a suitable BTI J.

* The current PAC-RET approach doesn't work for ILP32.

* There are no compatibility concerns with the patch, since this is
  essentially a private interface between libgcc and gcc.  (And in
  particular, we don't support building libgcc with any version of
  GCC except the one supplied with it.)

* Although glibc does have its own unwinder, it's only there to maintain
  compatiblity with ancient versions.  Those versions long predate aarch64
  support, so that unwinder isn't built for aarch64.  (And even if it were,
  it would expect the traditional interface instead of the current PAC one.)

The patch does have the potential to lower protection for processors
that implement PAC without BTI.  However, there's not much we can
reasonably do about that without creating a separate PAC-only mode
for libgcc.  And it isn't clear how useful such a mode would be.
A key feature of both PAC and BTI is that they use the NOP space,
and so it is safe to build PAC- and BTI-compatible binaries without
worrying about whether the target h/w supports them.  In particular,
even though PAC is a v8.3 feature and BTI is only a (backportable)
v8.5 feature, it is safe and useful to build v8.3 binaries with BTI
unconditionally.  Not many people are likely to want to compile-out
the extra BTI protection.

Given all the above, I agree that this is a good change to make.
And like you say, the affected interfaces are all specific to AArch64.

> This patch changes the unwinder hooks as follows:
>
> MD_POST_EXTRACT_ROOT_ADDR is removed: root address comes from
> __builtin_return_address which is not mangled.
>
> MD_POST_EXTRACT_FRAME_ADDR is renamed to MD_DEMANGLE_RETURN_ADDR,
> it now operates on _Unwind_Word instead of void *, so the hook
> should work when return address signing is enabled on AArch64 ilp32.
> (But for that __builtin_aarch64_autia1716 should be fixed to operate
> on 64bit input instead of a void *.)
>
> MD_POST_FROB_EH_HANDLER_ADDR is removed: it is the responsibility of
> __builtin_eh_return to do the mangling if necessary.
>
> libgcc/ChangeLog:
>
> 2020-06-04  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	* config/aarch64/aarch64-unwind.h (MD_POST_EXTRACT_ROOT_ADDR): Remove.
> 	(MD_POST_FROB_EH_HANDLER_ADDR): Remove.
> 	(MD_POST_EXTRACT_FRAME_ADDR): Rename to ...
> 	(MD_DEMANGLE_RETURN_ADDR): This.
> 	(aarch64_post_extract_frame_addr): Rename to ...
> 	(aarch64_demangle_return_addr): This.
> 	(aarch64_post_frob_eh_handler_addr): Remove.
> 	* unwind-dw2.c (uw_update_context): Demangle return address.
> 	(uw_frob_return_addr): Remove.

OK for trunk and branches, with a minor fix…

> @@ -57,9 +54,10 @@ aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
>     using CFA of current frame.  */
>  
>  static inline void *
> -aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
> -				 _Unwind_FrameState *fs, void *addr)
> +aarch64_demangle_return_addr (struct _Unwind_Context *context,
> +			      _Unwind_FrameState *fs, _Unwind_Word addr_word)

The function comment should now refer to ADDR_WORD instead of ADDR.

Thanks,
Richard
diff mbox series

Patch

diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
index ed84a96db41..b1d732e0b2d 100644
--- a/libgcc/config/aarch64/aarch64-unwind.h
+++ b/libgcc/config/aarch64/aarch64-unwind.h
@@ -27,11 +27,8 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #define DWARF_REGNUM_AARCH64_RA_STATE 34
 
-#define MD_POST_EXTRACT_ROOT_ADDR(addr)  __builtin_aarch64_xpaclri (addr)
-#define MD_POST_EXTRACT_FRAME_ADDR(context, fs, addr) \
-  aarch64_post_extract_frame_addr (context, fs, addr)
-#define MD_POST_FROB_EH_HANDLER_ADDR(current, target, addr) \
-  aarch64_post_frob_eh_handler_addr (current, target, addr)
+#define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
+  aarch64_demangle_return_addr (context, fs, addr)
 #define MD_FROB_UPDATE_CONTEXT(context, fs) \
   aarch64_frob_update_context (context, fs)
 
@@ -57,9 +54,10 @@  aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
    using CFA of current frame.  */
 
 static inline void *
-aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
-				 _Unwind_FrameState *fs, void *addr)
+aarch64_demangle_return_addr (struct _Unwind_Context *context,
+			      _Unwind_FrameState *fs, _Unwind_Word addr_word)
 {
+  void *addr = (void *)addr_word;
   if (context->flags & RA_SIGNED_BIT)
     {
       _Unwind_Word salt = (_Unwind_Word) context->cfa;
@@ -71,28 +69,6 @@  aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
     return addr;
 }
 
-/* Do AArch64 private frob on exception handler's address HANDLER_ADDR before
-   installing it into current context CURRENT.  TARGET is currently not used.
-   We need to sign exception handler's address if CURRENT itself is signed.  */
-
-static inline void *
-aarch64_post_frob_eh_handler_addr (struct _Unwind_Context *current,
-				   struct _Unwind_Context *target
-				   ATTRIBUTE_UNUSED,
-				   void *handler_addr)
-{
-  if (current->flags & RA_SIGNED_BIT)
-    {
-      if (aarch64_cie_signed_with_b_key (current))
-	return __builtin_aarch64_pacib1716 (handler_addr,
-					    (_Unwind_Word) current->cfa);
-      return __builtin_aarch64_pacia1716 (handler_addr,
-					(_Unwind_Word) current->cfa);
-    }
-  else
-    return handler_addr;
-}
-
 /* Do AArch64 private initialization on CONTEXT based on frame info FS.  Mark
    CONTEXT as return address signed if bit 0 of DWARF_REGNUM_AARCH64_RA_STATE is
    set.  */
diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index 62d4a3d29a2..fe896565d2e 100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -1538,11 +1538,14 @@  uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
     {
       /* Compute the return address now, since the return address column
 	 can change from frame to frame.  */
-      context->ra = __builtin_extract_return_addr
-	(_Unwind_GetPtr (context, fs->retaddr_column));
-#ifdef MD_POST_EXTRACT_FRAME_ADDR
-      context->ra = MD_POST_EXTRACT_FRAME_ADDR (context, fs, context->ra);
+      void *ret_addr;
+#ifdef MD_DEMANGLE_RETURN_ADDR
+      _Unwind_Word ra = _Unwind_GetGR (context, fs->retaddr_column);
+      ret_addr = MD_DEMANGLE_RETURN_ADDR (context, fs, ra);
+#else
+      ret_addr = _Unwind_GetPtr (context, fs->retaddr_column);
 #endif
+      context->ra = __builtin_extract_return_addr (ret_addr);
     }
 }
 
@@ -1577,9 +1580,6 @@  uw_init_context_1 (struct _Unwind_Context *context,
 		   void *outer_cfa, void *outer_ra)
 {
   void *ra = __builtin_extract_return_addr (__builtin_return_address (0));
-#ifdef MD_POST_EXTRACT_ROOT_ADDR
-  ra = MD_POST_EXTRACT_ROOT_ADDR (ra);
-#endif
   _Unwind_FrameState fs;
   _Unwind_SpTmp sp_slot;
   _Unwind_Reason_Code code;
@@ -1616,9 +1616,6 @@  uw_init_context_1 (struct _Unwind_Context *context,
      initialization context, then we can't see it in the given
      call frame data.  So have the initialization context tell us.  */
   context->ra = __builtin_extract_return_addr (outer_ra);
-#ifdef MD_POST_EXTRACT_ROOT_ADDR
-  context->ra = MD_POST_EXTRACT_ROOT_ADDR (context->ra);
-#endif
 }
 
 static void _Unwind_DebugHook (void *, void *)
@@ -1641,21 +1638,6 @@  _Unwind_DebugHook (void *cfa __attribute__ ((__unused__)),
 #endif
 }
 
-/* Frob exception handler's address kept in TARGET before installing into
-   CURRENT context.  */
-
-static inline void *
-uw_frob_return_addr (struct _Unwind_Context *current
-		     __attribute__ ((__unused__)),
-		     struct _Unwind_Context *target)
-{
-  void *ret_addr = __builtin_frob_return_addr (target->ra);
-#ifdef MD_POST_FROB_EH_HANDLER_ADDR
-  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
-#endif
-  return ret_addr;
-}
-
 /* Install TARGET into CURRENT so that we can return to it.  This is a
    macro because __builtin_eh_return must be invoked in the context of
    our caller.  FRAMES is a number of frames to be unwind.
@@ -1667,7 +1649,7 @@  uw_frob_return_addr (struct _Unwind_Context *current
   do									\
     {									\
       long offset = uw_install_context_1 ((CURRENT), (TARGET));		\
-      void *handler = uw_frob_return_addr ((CURRENT), (TARGET));	\
+      void *handler = __builtin_frob_return_addr ((TARGET)->ra);	\
       _Unwind_DebugHook ((TARGET)->cfa, handler);			\
       _Unwind_Frames_Extra (FRAMES);					\
       __builtin_eh_return (offset, handler);				\