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

Andrew Burgess
2025-08-14T15:51:31Z

"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