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 2B2C1394883F for ; Mon, 5 Dec 2022 19:04:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2B2C1394883F 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 858EC23A; Mon, 5 Dec 2022 11:04:43 -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 D4E693F885; Mon, 5 Dec 2022 11:04:33 -0800 (PST) From: Richard Sandiford To: Wilco Dijkstra Mail-Followup-To: Wilco Dijkstra ,GCC Patches , Szabolcs Nagy , richard.sandiford@arm.com Cc: GCC Patches , Szabolcs Nagy Subject: Re: [PATCH] libgcc: Fix uninitialized RA signing on AArch64 [PR107678] References: Date: Mon, 05 Dec 2022 19:04:32 +0000 In-Reply-To: (Wilco Dijkstra's message of "Thu, 1 Dec 2022 16:55:16 +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=-39.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,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: > 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 use REG_SAVED_OFFSET > as the state where the return address signing bit is valid, and if the > state is REG_UNSAVED, initialize it to 0. > > Passes bootstrap & regress, OK for commit? > > libgcc/ > PR target/107678 > * unwind-dw2.c (execute_cfa_program): Initialize offset of > DWARF_REGNUM_AARCH64_RA_STATE if in REG_UNSAVED state. > * config/aarch64/aarch64-unwind.h (aarch64_frob_update_contex): > Check state is REG_SAVED_OFFSET before using offset for RA state. > > --- > > diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h > index 26db9cbd9e5c526e0c410a4fc6be2bedb7d261cf..597133b3d708a50a366c8bfeff57475f5522b3f6 100644 > --- a/libgcc/config/aarch64/aarch64-unwind.h > +++ b/libgcc/config/aarch64/aarch64-unwind.h > @@ -71,21 +71,15 @@ aarch64_demangle_return_addr (struct _Unwind_Context *context, > } > > /* 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. */ > + CONTEXT as having a signed return address if DWARF_REGNUM_AARCH64_RA_STATE > + is initialized (REG_SAVED_OFFSET state) and the offset has bit 0 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. */ > + if (fs->regs.how[DWARF_REGNUM_AARCH64_RA_STATE] == REG_SAVED_OFFSET > + && (fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset & 1) != 0) > context->flags |= RA_SIGNED_BIT; > else > context->flags &= ~RA_SIGNED_BIT; Hmm, but the point of the original patch was to support code generators that emit DW_CFA_val_expression instead of DW_CFA_AARCH64_negate_ra_state. Doesn't this patch undo that? Also, if I understood correctly, the reason we use REG_UNSAVED is to ensure that state from one frame isn't carried across to a parent frame, in cases where the parent frame lacks any signing. That is, each frame should start out with a zero bit even if a child frame is unwound while it has a set bit. Thanks, Richard > diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c > index eaceace20298b9b13344aff9d1fe9ee5f9c7bd73..87f2ae065b67982ce48f74e45523d9c754a7661c 100644 > --- a/libgcc/unwind-dw2.c > +++ b/libgcc/unwind-dw2.c > @@ -1203,11 +1203,16 @@ execute_cfa_program (const unsigned char *insn_ptr, > > 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. */ > - reg = DWARF_REGNUM_AARCH64_RA_STATE; > - gcc_assert (fs->regs.how[reg] == REG_UNSAVED); > - fs->regs.reg[reg].loc.offset ^= 1; > + /* This CFA is multiplexed with Sparc. On AArch64 it's used to toggle > + the return address signing status. It is initialized at the first > + use and the state is stored in bit 0 of the offset. */ > + reg = DWARF_REGNUM_AARCH64_RA_STATE; > + if (fs->regs.how[reg] == REG_UNSAVED) > + { > + fs->regs.how[reg] = REG_SAVED_OFFSET; > + fs->regs.reg[reg].loc.offset = 0; > + } > + fs->regs.reg[reg].loc.offset ^= 1; > #else > /* ??? Hardcoded for SPARC register window configuration. */ > if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)