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

[Andrew Burgess <aburgess@redhat.com>]

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> 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.  

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 <aburgess@redhat.com>

Thanks,
Andrew