From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id zSS6KgMGnmg7YgMAWB0awg (envelope-from ) for ; Thu, 14 Aug 2025 11:51:31 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=bONQe/kV; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 989CF1E0B3; Thu, 14 Aug 2025 11:51:31 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 76E7B1E04C for ; Thu, 14 Aug 2025 11:51:30 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 109DD3858C2F for ; Thu, 14 Aug 2025 15:51:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 109DD3858C2F Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=bONQe/kV Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id DE2913858C42 for ; Thu, 14 Aug 2025 15:50:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DE2913858C42 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org DE2913858C42 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755186649; cv=none; b=HARJGCwx+2TqeEFAR8OUy/hdt9c2SMTGdh/syVX2d3irZ9R0sEl4Xq2x6Mi2UwGzWDHUgw0NdF9AbU2zDBYUZgpeOxJ/1Qw+R/eXcTj47QIP9wwxdshuSA9VyxQa3IMsWLJ8QydybpPzCpqTtxyAk/auyX4Ht3SE4+W67VtBubo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755186649; c=relaxed/simple; bh=br9SweUECUflrcnjn6GSs1/WLi0w1Nb1fy5crjiS++M=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=jIuMLbZKZIHG8WLOZ6m9SuOyqC8ge024/XqJkldp1L/8ZSQtgnP9l64yC3gb5EsREFq7rEIdzRZvyYvQqPgkl0c/F/rmwUNw1YZHbjXPsyO62L21lV2TRcbkBBKGcI1qOM9wQpc1s392sT9EmdV98TGo6X6PnOlBCp5MBsMnkhA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DE2913858C42 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1755186648; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=FMmJ4Ka/HoAjibmDyXyWVQgwHSzSIYtTWM6BuRI/fpQ=; b=bONQe/kVnMbqN4XxGddVyKmCjK/wKllWPLhZLPllMTn0aPeBRUJjsKAlVkBOiApPwmBSSr U74mDChR319AtzGq7og+7zxpSNJzKYakQPSdLl+XixEf5ZlobA5eM6RRiD2OnN7T2zdIbj 03kQyCIr3fltC15j/AzXQsAxG9vis7o= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-178-jjkcQFQhOFW9DT4dZrbJGg-1; Thu, 14 Aug 2025 11:50:46 -0400 X-MC-Unique: jjkcQFQhOFW9DT4dZrbJGg-1 X-Mimecast-MFC-AGG-ID: jjkcQFQhOFW9DT4dZrbJGg_1755186646 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-3b9d41cec2cso819241f8f.0 for ; Thu, 14 Aug 2025 08:50:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1755186645; x=1755791445; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=FMmJ4Ka/HoAjibmDyXyWVQgwHSzSIYtTWM6BuRI/fpQ=; b=LRqrY4u95dbhL1Bp/SkkjGeNhaTK1L7laiR2AMseHtmtIbYIePNvscVLXam6TUq52U 0/VKGgNf4/IoBzbrI/yfoJHcHiL4QEEVbkXr0QrGc1YRCsOJv3bb6FY/QR2eaNHKYe2V rMMQJldUbKMhyP3KCTZc3+bZ+YmkLK7mvUBhh+BGiel99wmfZeKo2zphpuB5uyHISp1q kN3hp9+Hp49Jm/RTz4GkG1D1p1NaRi73DmN/8q7SEpdwdyz0y+tOjlV4wnEx7VjwoHZD CpItpqbnz9P4ZmjGesMQapPYJBUGP4AYhYIA62CvVcfkQQVuqy75wdhdM/gu5LK3mbNq v5kg== X-Forwarded-Encrypted: i=1; AJvYcCWLeAGibp6+J6SnVOsOwyfUpsOf8gPCaBnyRzyEDfoGNjgr7aC25DLVfnG3UeynQ6mPa497lXkEE0Bctg==@sourceware.org X-Gm-Message-State: AOJu0YydfLwNdEMMwW6N9E+zGxOBlIjfJ0XbJ7KvHT+v+/c5m2zyEM1O QH7tHE9FaXiKfLbG1l0+4Tiz/DNPLW9fboD0+xINW22iVUG7DGm+4Pp/XK9mYfPR8Wq3xexotRO Vt9zqJusc8BM3hMqf8dCJg4ho1/mjvuv8PQOES33leHXpMF/s7ao6apFjevJp12c= X-Gm-Gg: ASbGnct+1TxUKLXXcVmxf7hMRJnIoqLeTl77LJEQaaJ82kXFj2vlTFYv4N+ghml66Iu vBbJGvIpzE59JG4tTUPnzNL5I1q0GXbbxei0EaQD1IcdboCfymvAZJLYTOozPbgQZg+7E9b7AXh qrokgHBqQqfBw9+31iXKKr/3YlfzGLJaIRCkaor7HItpoFkDiKSSlk0AG26jqSbM7kww0vjOYVm lhXv2qaMnZHLpvI54qiAfDo3HMgS/A/qjjVJA3eJ0Pg0uw0zJroUR8rxc2ui96t1g8dFVk3PPTa yDVnXolv7r+zX5YI9pdd9ydcoDerCpzowb49oT0/4UUwDihVYwpJUgQa5Nw= X-Received: by 2002:a05:6000:2504:b0:3b7:931d:3789 with SMTP id ffacd0b85a97d-3b9edfcc8a4mr2783592f8f.4.1755186645514; Thu, 14 Aug 2025 08:50:45 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEJsZNLCByvuAmgDyxQvWyYFtQM8T5mjBuo6ORfyk5ES3e4WilbVl1k0pghGRXZO4wE3sqUfg== X-Received: by 2002:a05:6000:2504:b0:3b7:931d:3789 with SMTP id ffacd0b85a97d-3b9edfcc8a4mr2783572f8f.4.1755186645040; Thu, 14 Aug 2025 08:50:45 -0700 (PDT) Received: from localhost (13.81.93.209.dyn.plus.net. [209.93.81.13]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3b79c4696c8sm50026229f8f.55.2025.08.14.08.50.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Aug 2025 08:50:44 -0700 (PDT) From: Andrew Burgess To: "Schimpe, Christina" , "gdb-patches@sourceware.org" Cc: "thiago.bauermann@linaro.org" , "luis.machado@arm.com" Subject: RE: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method to get the shadow stack pointer. In-Reply-To: References: <20250628082810.332526-1-christina.schimpe@intel.com> <20250628082810.332526-12-christina.schimpe@intel.com> <87cy9hdg1y.fsf@redhat.com> Date: Thu, 14 Aug 2025 16:50:43 +0100 Message-ID: <87ikip9a24.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 7rGcsY2yZPQaNnFk3XJP1_pfOFpIRcG10JyZam2QlPY_1755186646 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org "Schimpe, Christina" writes: > Hi Andrew, > > Thanks for the feedback. Please find my comments to your feedback below. > >> -----Original Message----- >> From: Andrew Burgess >> Sent: Wednesday, July 30, 2025 2:22 PM >> To: Schimpe, Christina ; gdb- >> patches@sourceware.org >> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com >> Subject: Re: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method to >> get the shadow stack pointer. >> >> Christina Schimpe writes: >> >> > This patch is required by the following commit >> > "gdb: Enable displaced stepping with shadow stack on amd64 linux." >> > >> > Reviewed-by: Thiago Jung Bauermann >> > Reviewed-By: Luis Machado >> > --- >> > gdb/arch-utils.c | 10 ++++++++++ >> > gdb/arch-utils.h | 5 +++++ >> > gdb/gdbarch-gen.c | 22 ++++++++++++++++++++++ >> > gdb/gdbarch-gen.h | 12 +++++++++++- >> > gdb/gdbarch_components.py | 17 ++++++++++++++++- >> > 5 files changed, 64 insertions(+), 2 deletions(-) >> > >> > diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index >> > f320d3d7365..c396e9e3840 100644 >> > --- a/gdb/arch-utils.c >> > +++ b/gdb/arch-utils.c >> > @@ -1218,6 +1218,16 @@ default_gdbarch_return_value >> > readbuf, writebuf); >> > } >> > >> > +/* See arch-utils.h. */ >> > + >> > +std::optional >> > +default_get_shadow_stack_pointer (gdbarch *gdbarch, regcache >> *regcache, >> > + bool &shadow_stack_enabled) >> > +{ >> > + shadow_stack_enabled = false; >> > + return {}; >> > +} >> > + >> > obstack *gdbarch_obstack (gdbarch *arch) { >> > return &arch->obstack; >> > diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h index >> > 1509cb7441e..14a84b74733 100644 >> > --- a/gdb/arch-utils.h >> > +++ b/gdb/arch-utils.h >> > @@ -414,4 +414,9 @@ extern enum return_value_convention >> default_gdbarch_return_value >> > struct regcache *regcache, struct value **read_value, >> > const gdb_byte *writebuf); >> > >> > +/* Default implementation of gdbarch default_get_shadow_stack_pointer >> > + method. */ >> > +extern std::optional default_get_shadow_stack_pointer >> > + (gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled); >> > + >> > #endif /* GDB_ARCH_UTILS_H */ >> > diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c index >> > a4b72793fd8..caeda3cefae 100644 >> > --- a/gdb/gdbarch-gen.c >> > +++ b/gdb/gdbarch-gen.c >> > @@ -263,6 +263,7 @@ struct gdbarch >> > gdbarch_use_target_description_from_corefile_notes_ftype >> *use_target_description_from_corefile_notes = >> default_use_target_description_from_corefile_notes; >> > gdbarch_core_parse_exec_context_ftype *core_parse_exec_context = >> default_core_parse_exec_context; >> > gdbarch_shadow_stack_push_ftype *shadow_stack_push = nullptr; >> > + gdbarch_get_shadow_stack_pointer_ftype *get_shadow_stack_pointer = >> > + default_get_shadow_stack_pointer; >> > }; >> > >> > /* Create a new ``struct gdbarch'' based on information provided by >> > @@ -537,6 +538,7 @@ verify_gdbarch (struct gdbarch *gdbarch) >> > /* Skip verify of use_target_description_from_corefile_notes, invalid_p >> == 0. */ >> > /* Skip verify of core_parse_exec_context, invalid_p == 0. */ >> > /* Skip verify of shadow_stack_push, has predicate. */ >> > + /* Skip verify of get_shadow_stack_pointer, invalid_p == 0. */ >> > if (!log.empty ()) >> > internal_error (_("verify_gdbarch: the following are invalid ...%s"), >> > log.c_str ()); >> > @@ -1414,6 +1416,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct >> ui_file *file) >> > gdb_printf (file, >> > "gdbarch_dump: shadow_stack_push = <%s>\n", >> > host_address_to_string (gdbarch->shadow_stack_push)); >> > + gdb_printf (file, >> > + "gdbarch_dump: get_shadow_stack_pointer = <%s>\n", >> > + host_address_to_string (gdbarch->get_shadow_stack_pointer)); >> > if (gdbarch->dump_tdep != NULL) >> > gdbarch->dump_tdep (gdbarch, file); } @@ -5583,3 +5588,20 @@ >> > set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch, { >> > gdbarch->shadow_stack_push = shadow_stack_push; } >> > + >> > +std::optional >> > +gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, regcache >> > +*regcache, bool &shadow_stack_enabled) { >> > + gdb_assert (gdbarch != NULL); >> > + gdb_assert (gdbarch->get_shadow_stack_pointer != NULL); >> > + if (gdbarch_debug >= 2) >> > + gdb_printf (gdb_stdlog, "gdbarch_get_shadow_stack_pointer >> > +called\n"); >> > + return gdbarch->get_shadow_stack_pointer (gdbarch, regcache, >> > +shadow_stack_enabled); } >> > + >> > +void >> > +set_gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, >> > + >> gdbarch_get_shadow_stack_pointer_ftype >> > +get_shadow_stack_pointer) { >> > + gdbarch->get_shadow_stack_pointer = get_shadow_stack_pointer; } >> > diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index >> > 71142332540..c36171b089e 100644 >> > --- a/gdb/gdbarch-gen.h >> > +++ b/gdb/gdbarch-gen.h >> > @@ -1807,7 +1807,8 @@ extern void >> set_gdbarch_core_parse_exec_context (struct gdbarch *gdbarch, gdbarc >> > technologies. For example, the Intel Control-Flow Enforcement >> Technology >> > (Intel CET) on x86 provides a shadow stack and indirect branch tracking. >> > To enable shadow stack support for inferior calls the >> shadow_stack_push >> > - gdbarch hook has to be provided. >> > + gdbarch hook has to be provided. The get_shadow_stack_pointer >> gdbarch >> > + hook has to be provided to enable displaced stepping. >> > >> > Push NEW_ADDR to the shadow stack and update the shadow stack >> > pointer. */ >> > >> > @@ -1816,3 +1817,12 @@ extern bool gdbarch_shadow_stack_push_p >> (struct >> > gdbarch *gdbarch); typedef void (gdbarch_shadow_stack_push_ftype) >> > (struct gdbarch *gdbarch, CORE_ADDR new_addr, regcache *regcache); >> > extern void gdbarch_shadow_stack_push (struct gdbarch *gdbarch, >> > CORE_ADDR new_addr, regcache *regcache); extern void >> > set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch, >> > gdbarch_shadow_stack_push_ftype *shadow_stack_push); >> > + >> > +/* If possible, return the shadow stack pointer. On some architectures, >> the >> > + shadow stack pointer is available even if the feature is disabled. To >> > + return the feature's enablement state configure >> SHADOW_STACK_ENABLED. >> > + Set it to true in case the shadow stack is enabled. */ >> > + >> > +typedef std::optional >> > +(gdbarch_get_shadow_stack_pointer_ftype) (struct gdbarch *gdbarch, >> > +regcache *regcache, bool &shadow_stack_enabled); extern >> > +std::optional gdbarch_get_shadow_stack_pointer (struct >> > +gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled); >> > +extern void set_gdbarch_get_shadow_stack_pointer (struct gdbarch >> > +*gdbarch, gdbarch_get_shadow_stack_pointer_ftype >> > +*get_shadow_stack_pointer); >> > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py >> > index abc79588473..73459064170 100644 >> > --- a/gdb/gdbarch_components.py >> > +++ b/gdb/gdbarch_components.py >> > @@ -2855,7 +2855,8 @@ Some targets support special hardware- >> assisted >> > control-flow protection technologies. For example, the Intel >> > Control-Flow Enforcement Technology (Intel CET) on x86 provides a >> shadow stack and indirect branch tracking. >> > To enable shadow stack support for inferior calls the >> > shadow_stack_push -gdbarch hook has to be provided. >> > +gdbarch hook has to be provided. The get_shadow_stack_pointer >> > +gdbarch hook has to be provided to enable displaced stepping. >> >> I find the addition of this last sentence here a little strange. While it's a true >> statement, wouldn't this be better placed on the comment for >> get_shadow_stack_pointer? > > Mh I see the initial comment section here rather as general overview for the > shadow stack feature in GDB. It explains which features (e.g. infcalls and displaced > stepping) are interacting with shadow stacks. So in my opinion it's okay to keep it as is. > >> > >> > Push NEW_ADDR to the shadow stack and update the shadow stack >> pointer. >> > """, >> > @@ -2864,3 +2865,17 @@ Push NEW_ADDR to the shadow stack and >> update the shadow stack pointer. >> > params=[("CORE_ADDR", "new_addr"), ("regcache *", "regcache")], >> > predicate=True, >> > ) >> > + >> > +Method( >> > + comment=""" >> > +If possible, return the shadow stack pointer. On some architectures, >> > +the shadow stack pointer is available even if the feature is >> > +disabled. To return the feature's enablement state configure >> SHADOW_STACK_ENABLED. >> > +Set it to true in case the shadow stack is enabled. >> >> The wording "configure SHADOW_STACK_ENABLED" seems a little strange. >> Also, there's a bunch of important detail that this comment doesn't cover. >> Here's what I'd suggest, though it's possible this doesn't match the >> implementation (I haven't checked the next patch yet), but this does match >> default_get_shadow_stack_pointer. Feel free to take any of this that is >> useful: >> >> If possible, return the shadow stack pointer. On some architectures, >> the shadow stack pointer is available even if the feature is disabled. >> If the shadow stack feature is enabled then set SHADOW_STACK_ENABLED >> to true, otherwise set SHADOW_STACK_ENABLED to false. The >> SHADOW_STACK_ENABLED will always be set if this function returns a >> value. If the function doesn't return a value then the state of >> SHADOW_STACK_ENABLED is undefined. > > Hm, I am not sure if I am missing something here. > > We set SHADOW_STACK_ENABLED to false in default_get_shadow_stack_pointer > and *do not* return a value. > ~~~ > std::optional > default_get_shadow_stack_pointer (gdbarch *gdbarch, regcache *regcache, > bool &shadow_stack_enabled) > { > shadow_stack_enabled = false; > return {}; > } > ~~~ > > What do you think about the following: > If possible, return the shadow stack pointer. If the shadow stack feature is enabled > then set SHADOW_STACK_ENABLED to true, otherwise set SHADOW_STACK_ENABLED > to false. > On some architectures, the shadow stack pointer is available even if the feature is disabled. > So dependent on the target, an implementation of this function may return a valid shadow > stack pointer, but set SHADOW_STACK_ENABLED to false. That's great. Could you also add a sentence similar to the one added to shadow_stack_push that get_shadow_stack_pointer must be implemented in order for displaced stepping to work. I still don't understand why you want to document that detail in the comment for shadow_stack_push, but I don't think it's going to do any harm. But I do think details about get_shadow_stack_pointer should be documented on get_shadow_stack_pointer! With that extra sentence added: Approved-By: Andrew Burgess Thanks, Andrew