RE: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method to get the shadow stack pointer.

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Andrew,

Thanks for the feedback. Please find my comments to your feedback below.

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, July 30, 2025 2:22 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; 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 <christina.schimpe@intel.com> writes:
> 
> > This patch is required by the following commit
> > "gdb: Enable displaced stepping with shadow stack on amd64 linux."
> >
> > Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> > Reviewed-By: Luis Machado <luis.machado@arm.com>
> > ---
> >  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<CORE_ADDR>
> > +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<CORE_ADDR> 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<CORE_ADDR>
> > +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<CORE_ADDR>
> > +(gdbarch_get_shadow_stack_pointer_ftype) (struct gdbarch *gdbarch,
> > +regcache *regcache, bool &shadow_stack_enabled); extern
> > +std::optional<CORE_ADDR> 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<CORE_ADDR>
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.  


> 
> > +""",
> > +    type="std::optional<CORE_ADDR>",
> > +    name="get_shadow_stack_pointer",
> > +    params=[("regcache *", "regcache"), ("bool &",
> "shadow_stack_enabled")],
> > +    predefault="default_get_shadow_stack_pointer",
> > +    invalid=False,
> > +)
> > --
> > 2.43.0

Kind Regards
Christina

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928