Re: [PATCH] libgcc: Fix uninitialized RA signing on AArch64 [PR107678]

[Richard Sandiford <richard.sandiford@arm.com>]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> Hi,
>
>> @Wilco, can you please send the rebased patch for patch review? We would
>> need in out openSUSE package soon.
>
> Here is an updated and rebased version:
>
> Cheers,
> Wilco
>
> v4: rebase and add REG_UNSAVED_ARCHEXT.
>
> A recent change only initializes the regs.how[] during Dwarf unwinding
> which resulted in an uninitialized offset used in return address signing
> and random failures during unwinding.  The fix is to encode the return
> address signing state in REG_UNSAVED and a new state REG_UNSAVED_ARCHEXT.
>
> Passes bootstrap & regress, OK for commit?
>
> libgcc/
>         PR target/107678
>         * unwind-dw2.h (REG_UNSAVED_ARCHEXT): Add new enum.
>         * unwind-dw2.c (uw_update_context_1): Add REG_UNSAVED_ARCHEXT case.
>         * unwind-dw2-execute_cfa.h: Use REG_UNSAVED_ARCHEXT/REG_UNSAVED to
>         encode the return address signing state.
>         * config/aarch64/aarch64-unwind.h (aarch64_demangle_return_addr)
>         Check current return address signing state.
>         (aarch64_frob_update_contex): Remove.
>
> ---
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index 874cf6c3e77fb72d999f51b636d74cb0b5728bbd..727c27ba5da983958b3134715d9d4d7c0af5c1e2 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -29,8 +29,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>
>  #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)
>
>  static inline int
>  aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
> @@ -55,42 +53,27 @@ aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
>
>  static inline void *
>  aarch64_demangle_return_addr (struct _Unwind_Context *context,
> -                             _Unwind_FrameState *fs ATTRIBUTE_UNUSED,
> +                             _Unwind_FrameState *fs,
>                               _Unwind_Word addr_word)
>  {
>    void *addr = (void *)addr_word;
> -  if (context->flags & RA_SIGNED_BIT)
> +  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
> +
> +  if (fs->regs.how[reg] == REG_UNSAVED)
> +    return addr;
> +
> +  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
> +     REG_UNDEFINED means enabled), or set by a DW_CFA_expression.  */

Needs updating to REG_UNSAVED_ARCHEXT.

OK with that changes, thanks, and sorry for the delays & runaround.

Richard

> +  if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
> +      || (_Unwind_GetGR (context, reg) & 0x1) != 0)
>      {
>        _Unwind_Word salt = (_Unwind_Word) context->cfa;
>        if (aarch64_cie_signed_with_b_key (context) != 0)
>         return __builtin_aarch64_autib1716 (addr, salt);
>        return __builtin_aarch64_autia1716 (addr, salt);
>      }
> -  else
> -    return 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.  */
> -
> -static inline void
> -aarch64_frob_update_context (struct _Unwind_Context *context,
> -                            _Unwind_FrameState *fs)
> -{
> -  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -  int ra_signed;
> -  if (fs->regs.how[reg] == REG_UNSAVED)
> -    ra_signed = fs->regs.reg[reg].loc.offset & 0x1;
> -  else
> -    ra_signed = _Unwind_GetGR (context, reg) & 0x1;
> -  if (ra_signed)
> -    /* The flag is used for re-authenticating EH handler's address.  */
> -    context->flags |= RA_SIGNED_BIT;
> -  else
> -    context->flags &= ~RA_SIGNED_BIT;
>
> -  return;
> +  return addr;
>  }
>
>  #endif /* defined AARCH64_UNWIND_H && defined __ILP32__ */
> diff --git a/libgcc/unwind-dw2-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
> index 264c11c03ec4a09cac2c19a241c5b110b1b6b602..aef377092ceede6bdda8532679f9b081c98fadce 100644
> --- a/libgcc/unwind-dw2-execute_cfa.h
> +++ b/libgcc/unwind-dw2-execute_cfa.h
> @@ -278,10 +278,15 @@
>         case DW_CFA_GNU_window_save:
>  #if defined (__aarch64__) && !defined (__ILP32__)
>           /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
> -            return address signing status.  */
> +            return address signing status.  REG_UNSAVED/REG_UNSAVED_ARCHEXT
> +            mean RA signing is disabled/enabled.  */
>           reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -         gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
> -         fs->regs.reg[reg].loc.offset ^= 1;
> +         gcc_assert (fs->regs.how[reg] == REG_UNSAVED
> +                     || fs->regs.how[reg] == REG_UNSAVED_ARCHEXT);
> +         if (fs->regs.how[reg] == REG_UNSAVED)
> +           fs->regs.how[reg] = REG_UNSAVED_ARCHEXT;
> +         else
> +           fs->regs.how[reg] = REG_UNSAVED;
>  #else
>           /* ??? Hardcoded for SPARC register window configuration.  */
>           if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
> index e2f81983e1dcf3df6aebde2454630b7bee87d597..53e1b183c7d60112a14411d3356c49cb39cd0de7 100644
> --- a/libgcc/unwind-dw2.h
> +++ b/libgcc/unwind-dw2.h
> @@ -29,6 +29,7 @@ enum {
>    REG_SAVED_EXP,
>    REG_SAVED_VAL_OFFSET,
>    REG_SAVED_VAL_EXP,
> +  REG_UNSAVED_ARCHEXT,         /* Target specific extension.  */
>    REG_UNDEFINED
>  };
>
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 9c5bf7821916d8ac8e10e25a7123cd03f848019a..d0afce7a9ea9f5b12a5a01ef1e940e1452b48cab 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -137,9 +137,6 @@ struct _Unwind_Context
>  #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
>    /* Context which has version/args_size/by_value fields.  */
>  #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
> -  /* Bit reserved on AArch64, return address has been signed with A or B
> -     key.  */
> -#define RA_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)
>    _Unwind_Word flags;
>    /* 0 for now, can be increased when further fields are added to
>       struct _Unwind_Context.  */
> @@ -1200,6 +1197,7 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>        {
>        case REG_UNSAVED:
>        case REG_UNDEFINED:
> +      case REG_UNSAVED_ARCHEXT:
>         break;
>
>        case REG_SAVED_OFFSET: