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

[Richard Sandiford]

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