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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

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

>> -----Original Message-----
>> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Sent: Saturday, February 8, 2025 5:04 AM
>> To: Schimpe, Christina <christina.schimpe@intel.com>
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to get the
>> shadow stack pointer.
>>
>>
>> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>>
>> >> -----Original Message-----
>> >> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> >> Sent: Thursday, February 6, 2025 4:35 AM
>> >> To: Schimpe, Christina <christina.schimpe@intel.com>
>> >> Cc: gdb-patches@sourceware.org
>> >> Subject: Re: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to
>> >> get the shadow stack pointer.
>> >>
>> >>
>> >> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>> >>
>> >> > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
>> >> > index 52f265e8e0e..df70cb082a4 100644
>> >> > --- a/gdb/gdbarch_components.py
>> >> > +++ b/gdb/gdbarch_components.py
>> >> > @@ -2822,6 +2822,8 @@ Some targets support special
>> >> > hardware-assisted control-flow protection  technologies.  For
>> >> > example, Intel's Control-flow Enforcement Technology (CET)
>> >> > provides a shadow stack and
>> >> indirect branch tracking.
>> >> >  To enable inferior calls the function shadow_stack_push has to be provided.
>> >> > +The method get_shadow_stack_pointer has to be provided to enable
>> >> > +displaced stepping.
>> >> >
>> >> >  Push the address NEW_ADDR on the shadow stack and update the
>> >> > shadow stack  pointer.
>> >> > @@ -2831,3 +2833,11 @@ pointer.
>> >> >      params=[("CORE_ADDR", "new_addr")],
>> >> >      predicate=True,
>> >> >  )
>> >> > +
>> >> > +Method(
>> >> > +    type="std::optional<CORE_ADDR>",
>> >> > +    name="get_shadow_stack_pointer",
>> >> > +    params=[],
>> >> > +    predefault="default_get_shadow_stack_pointer",
>> >> > +    invalid=False,
>> >> > +)
>> >>
>> >> Ideally, there should be a comment on this method entry.
>> >>
>> >> This method is only used in amd64-tdep.c and i386-tdep.c. IMHO it
>> >> would be better to put it in i386_gdbarch_tdep instead.
>> >
>> > Hi Thiago,
>> >
>> > Thank you for the review.
>> >
>> > As also discussed here:
>> > https://sourceware.org/pipermail/gdb-patches/2025-February/215266.html
>> > , I wonder if it's better to keep the code generic.
>>
>> Indeed, that's a good point. In the case of this method, I have an additional
>> concern though: its callers assume that if gdbarch_get_shadow_stack_pointer
>> returns a value, then it means that shadow stacks are enabled in the inferior. But
>> this is an x86 particularity. On AArch64, if the processor supports shadow stacks
>> then the shadow stack register is always available, even if shadow stacks are
>> turned off.
>
> Ah ok, I did not know.
>
>> So I think that there should be an additional method to indicate whether shadow
>> stacks are enabled in the inferior.
>
> Yes, that makes sense. But I want to avoid that we call ptrace twice on x86,  once in the method
> to check the enablement state and once to get the shadow stack pointer.

Ok, makes sense.

> Would something like that be acceptable as well?
>
> 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 shadow stack
> enablement state configure SHADOW_STACK_ENABLED.
>   """,
> type="std::optional<CORE_ADDR>",
> name="get_shadow_stack_pointer",
> params=[("bool &", "shadow_stack_enabled")],
> predefault="default_get_shadow_stack_pointer",
> invalid=False,
> )

Yes, this looks good.

--
Thiago