From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 4B9A33858D28 for ; Tue, 17 Jan 2023 20:43:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4B9A33858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 21CEB1477; Tue, 17 Jan 2023 12:44:20 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 24A893F71A; Tue, 17 Jan 2023 12:43:37 -0800 (PST) From: Richard Sandiford To: Wilco Dijkstra Mail-Followup-To: Wilco Dijkstra ,Martin =?utf-8?Q?Li=C5=A1ka?= , Jakub Jelinek , Szabolcs Nagy , GCC Patches , richard.sandiford@arm.com Cc: Martin =?utf-8?Q?Li=C5=A1ka?= , Jakub Jelinek , Szabolcs Nagy , GCC Patches Subject: Re: [PATCH] libgcc: Fix uninitialized RA signing on AArch64 [PR107678] References: Date: Tue, 17 Jan 2023 20:43:35 +0000 In-Reply-To: (Wilco Dijkstra's message of "Tue, 17 Jan 2023 19:49:39 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-37.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Wilco Dijkstra 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: