From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 436113858400 for ; Thu, 22 Sep 2022 12:59:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 436113858400 Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id EB7C28758D; Thu, 22 Sep 2022 12:59:48 +0000 (UTC) Date: Thu, 22 Sep 2022 12:59:38 +0000 From: Lancelot SIX To: Luis Machado Cc: gdb-patches@sourceware.org, jhb@FreeBSD.org, thiago.bauermann@linaro.org Subject: Re: [PATCH,v3] [aarch64] Fix removal of non-address bits for PAuth Message-ID: <20220922125805.hvekyxcf3nc2i764@ubuntu.lan> References: <20220705140037.135012-1-luis.machado@arm.com> <20220823202936.1561526-1-luis.machado@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220823202936.1561526-1-luis.machado@arm.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Thu, 22 Sep 2022 12:59:49 +0000 (UTC) X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_SBL_CSS, SPF_HELO_NONE, SPF_PASS, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Sep 2022 12:59:52 -0000 Hi Luis, I went through the patch and have a couple of questions above. > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 15773c75da8..279c8d98f5d 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -1787,7 +1787,8 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch, > uiout->text ("\n"); > > gdb::optional atag > - = aarch64_mte_get_atag (address_significant (gdbarch, fault_addr)); > + = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, > + fault_addr)); > gdb_byte ltag = aarch64_mte_get_ltag (fault_addr); > > if (!atag.has_value ()) > @@ -1961,6 +1962,47 @@ aarch64_linux_decode_memtag_section (struct gdbarch *gdbarch, > return tags; > } > > +/* AArch64 implementation of the remove_non_address_bits gdbarch hook. Remove > + non address bits from a pointer value. */ > + > +static CORE_ADDR > +aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer) > +{ > + aarch64_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + > + /* By default, we assume TBI and discard the top 8 bits plus the VA range > + select bit (55). */ > + CORE_ADDR mask = AARCH64_TOP_BITS_MASK; > + > + if (tdep->has_pauth ()) > + { > + /* Fetch the PAC masks. These masks are per-process, so we can just > + fetch data from whatever thread we have at the moment. > + > + Also, we have both a code mask and a data mask. For now they are the > + same, but this may change in the future. */ > + struct regcache *regs = get_current_regcache (); > + CORE_ADDR cmask, dmask; > + > + if (regs->cooked_read (tdep->pauth_reg_base, &dmask) != REG_VALID) > + dmask = mask; > + > + if (regs->cooked_read (tdep->pauth_reg_base + 1, &cmask) != REG_VALID) > + cmask = mask; > + > + if (dmask != cmask) > + { > + /* Warn if the masks are different. */ > + aarch64_pauth_mask_warning (); > + mask |= dmask > cmask? dmask : cmask; > + } > + else > + mask |= cmask; Here, I am wondering what happens if either cooked_read does not return ROG_VALID. Wouldn't cmask/dmask have un-initialized values, making the end of the method hazardous? I guess initializing both to 0 would solve this. > + } > + > + return aarch64_remove_top_bits (pointer, mask); > +} > + > static void > aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > { > index 0f73286f145..d9c4b994850 100644 > --- a/gdb/arch/aarch64.c > +++ b/gdb/arch/aarch64.c > @@ -58,3 +58,30 @@ aarch64_create_target_description (const aarch64_features &features) > > return tdesc.release (); > } > + > +/* See arch/aarch64.h. */ > + > +CORE_ADDR > +aarch64_remove_top_bits (CORE_ADDR pointer, CORE_ADDR mask) > +{ > + /* The VA range select bit is 55. This bit tells us if we have a > + kernel-space address or a user-space address. */ > + bool kernel_address = (pointer & VA_RANGE_SELECT_BIT_MASK) != 0; > + I am wondering: is this Linux specific or is this valid accross all configurations? If this is linux specific, is aarch64.c the right place to implement this? Best, Lancelot. > + /* Remove the top non-address bits. */ > + pointer &= ~mask; > + > + /* Sign-extend if we have a kernel-space address. */ > + if (kernel_address) > + pointer |= mask; > + > + return pointer; > +} > + > +/* See arch/aarch64.h. */ > + > +void > +aarch64_pauth_mask_warning () > +{ > + warning (_("Pointer authentication masks for code (C) and data (D) differ")); > +}