diff mbox series

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

Message ID 20241220200501.324191-12-christina.schimpe@intel.com
State New
Headers
Series Add CET shadow stack support |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Schimpe, Christina Dec. 20, 2024, 8:05 p.m. UTC 
  This patch is required by the following commit.
---
 gdb/arch-utils.c          |  8 ++++++++
 gdb/arch-utils.h          |  5 +++++
 gdb/gdbarch-gen.c         | 22 ++++++++++++++++++++++
 gdb/gdbarch-gen.h         |  6 ++++++
 gdb/gdbarch_components.py | 10 ++++++++++
 5 files changed, 51 insertions(+)

Comments

Guinevere Larsen Jan. 28, 2025, 8:27 p.m. UTC | #1 
On 12/20/24 5:05 PM, Schimpe, Christina wrote:
> This patch is required by the following commit.
> ---

Maybe others disagree (in which case go with their suggestion), but my 
opinion is that, because you're only adding gdbarch functionality but no 
one uses it yet, there's basically no chance this commit could 
inadvertently break something, so it is fine to merge with the next one 
where you make use of the feature.

If these are split, I would like to have some explanation as to why it 
is required, but I imagine the explanation finds a better home in the 
next commit anyway, which is another reason why I'd join both commits.
Luis Machado Jan. 30, 2025, 10:33 a.m. UTC | #2 
On 1/28/25 20:27, Guinevere Larsen wrote:
> On 12/20/24 5:05 PM, Schimpe, Christina wrote:
>> This patch is required by the following commit.
>> ---
> 
> Maybe others disagree (in which case go with their suggestion), but my opinion is that, because you're only adding gdbarch functionality but no one uses it yet, there's basically no chance this commit could inadvertently break something, so it is fine to merge with the next one where you make use of the feature.
> 
> If these are split, I would like to have some explanation as to why it is required, but I imagine the explanation finds a better home in the next commit anyway, which is another reason why I'd join both commits.
> 

As a counterpoint, I suppose it is fine to do it this way for simplicity of reviewing, as it is broken up into smaller bits. Things will potentially be pushed as part of a series anyway.
Schimpe, Christina Jan. 30, 2025, 12:34 p.m. UTC | #3 
> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Thursday, January 30, 2025 11:34 AM
> To: Guinevere Larsen <guinevere@redhat.com>; Schimpe, Christina
> <christina.schimpe@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to get the
> shadow stack pointer.
> 
> On 1/28/25 20:27, Guinevere Larsen wrote:
> > On 12/20/24 5:05 PM, Schimpe, Christina wrote:
> >> This patch is required by the following commit.
> >> ---
> >
> > Maybe others disagree (in which case go with their suggestion), but my opinion
> is that, because you're only adding gdbarch functionality but no one uses it yet,
> there's basically no chance this commit could inadvertently break something, so it
> is fine to merge with the next one where you make use of the feature.
> >
> > If these are split, I would like to have some explanation as to why it is required,
> but I imagine the explanation finds a better home in the next commit anyway,
> which is another reason why I'd join both commits.
> >
> 
> As a counterpoint, I suppose it is fine to do it this way for simplicity of reviewing,
> as it is broken up into smaller bits. Things will potentially be pushed as part of a
> series anyway.

Thanks for the feedback!

In my opinion, splitting the commits simplifies the review, especially if the commits are bigger. 
Also I thought it's good to keep target independent code separate.

For the commit " gdb, gdbarch: Enable inferior calls for shadow stack support.", for instance,  
I do not have 2 commits for the gdbarch function and it's usage, as in this case the gdbarch
function is called in target independent code only. I only have the implementation in a separate
commit "gdb: Implement amd64 linux shadow stack support for inferior calls.".

But I understand Guinevere's suggestion here, especially because the commit that uses
gdbarch_get_shadow_stack_pointer is only called in a patch which isn't really too big to review.

So I have mixed feelings about that and would be happy about a guideline. 

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
Guinevere Larsen Jan. 30, 2025, 1:42 p.m. UTC | #4 
On 1/30/25 9:34 AM, Schimpe, Christina wrote:
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: Thursday, January 30, 2025 11:34 AM
>> To: Guinevere Larsen <guinevere@redhat.com>; Schimpe, Christina
>> <christina.schimpe@intel.com>; gdb-patches@sourceware.org
>> Subject: Re: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to get the
>> shadow stack pointer.
>>
>> On 1/28/25 20:27, Guinevere Larsen wrote:
>>> On 12/20/24 5:05 PM, Schimpe, Christina wrote:
>>>> This patch is required by the following commit.
>>>> ---
>>> Maybe others disagree (in which case go with their suggestion), but my opinion
>> is that, because you're only adding gdbarch functionality but no one uses it yet,
>> there's basically no chance this commit could inadvertently break something, so it
>> is fine to merge with the next one where you make use of the feature.
>>> If these are split, I would like to have some explanation as to why it is required,
>> but I imagine the explanation finds a better home in the next commit anyway,
>> which is another reason why I'd join both commits.
>> As a counterpoint, I suppose it is fine to do it this way for simplicity of reviewing,
>> as it is broken up into smaller bits. Things will potentially be pushed as part of a
>> series anyway.
> Thanks for the feedback!
>
> In my opinion, splitting the commits simplifies the review, especially if the commits are bigger.
> Also I thought it's good to keep target independent code separate.
>
> For the commit " gdb, gdbarch: Enable inferior calls for shadow stack support.", for instance,
> I do not have 2 commits for the gdbarch function and it's usage, as in this case the gdbarch
> function is called in target independent code only. I only have the implementation in a separate
> commit "gdb: Implement amd64 linux shadow stack support for inferior calls.".
>
> But I understand Guinevere's suggestion here, especially because the commit that uses
> gdbarch_get_shadow_stack_pointer is only called in a patch which isn't really too big to review.
>
> So I have mixed feelings about that and would be happy about a guideline.

Yeah having had the time to read patch 12, I do think they'd go best 
together, but I don't know if there is any unified guidance on this.

Whatever you decide is fine, though, I don't have too strong an opinion 
on this.
Thiago Jung Bauermann Feb. 6, 2025, 3:35 a.m. UTC | #5 
"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.

--
Thiago
Schimpe, Christina Feb. 7, 2025, 12:01 p.m. UTC | #6 
> -----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.

And I plan to use the gdbarch method gdbarch_get_shadow_stack_pointer in a future series for "bt shadow".
I shared the commits in this comment:
https://sourceware.org/pipermail/gdb-patches/2025-February/215178.html,

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
Thiago Jung Bauermann Feb. 8, 2025, 4:03 a.m. UTC | #7 
"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.

So I think that there should be an additional method to indicate whether
shadow stacks are enabled in the inferior.

> And I plan to use the gdbarch method gdbarch_get_shadow_stack_pointer in a future series
> for "bt shadow".
> I shared the commits in this comment:
> https://sourceware.org/pipermail/gdb-patches/2025-February/215178.html,

Indeed. I see the same assumption in that code as well. Which makes a
lot of sense since it only needs to support x86. But that will need to
be adapted when posted upstream.

--
Thiago
Schimpe, Christina Feb. 10, 2025, 8:58 a.m. UTC | #8 
> -----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.
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,
)

> > And I plan to use the gdbarch method gdbarch_get_shadow_stack_pointer
> > in a future series for "bt shadow".
> > I shared the commits in this comment:
> > https://sourceware.org/pipermail/gdb-patches/2025-February/215178.html
> > ,
> 
> Indeed. I see the same assumption in that code as well. Which makes a lot of
> sense since it only needs to support x86. But that will need to be adapted when
> posted upstream.

Yes, I agree.

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
Thiago Jung Bauermann Feb. 11, 2025, 1:53 a.m. UTC | #9 
"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
Thiago Jung Bauermann Feb. 15, 2025, 3:45 a.m. UTC | #10 
Hello Christina,

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>
>> 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.

I was rebasing my Guarded Control Stack code on top of this series, and
noticed that I will need to access the thread's regcache for the aarch64
implementation of this gdbarch method. Then I noticed that the x86_64
version also needs it, and calls "get_thread_regcache (inferior_thread ())".
It would be better to pass the regcache as an argument instead.

--
Thiago
Schimpe, Christina Feb. 16, 2025, 10:45 a.m. UTC | #11 
> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Saturday, February 15, 2025 4:45 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.
> 
> Hello Christina,
> 
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> 
> > "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> >
> >> 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.
> 
> I was rebasing my Guarded Control Stack code on top of this series, and
> noticed that I will need to access the thread's regcache for the aarch64
> implementation of this gdbarch method. Then I noticed that the x86_64
> version also needs it, and calls "get_thread_regcache (inferior_thread ())".
> It would be better to pass the regcache as an argument instead.

Hi Thiago, 

Do you mean instead or in addition?
I thought you also need the parameter ("bool &", "shadow_stack_enabled").

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
Schimpe, Christina Feb. 20, 2025, 8:48 a.m. UTC | #12 
> -----Original Message-----
> From: Schimpe, Christina
> Sent: Sunday, February 16, 2025 11:45 AM
> To: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to get
> the shadow stack pointer.
> 
> > -----Original Message-----
> > From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> > Sent: Saturday, February 15, 2025 4:45 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.
> >
> > Hello Christina,
> >
> > Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> >
> > > "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> > >
> > >> 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.
> >
> > I was rebasing my Guarded Control Stack code on top of this series,
> > and noticed that I will need to access the thread's regcache for the
> > aarch64 implementation of this gdbarch method. Then I noticed that the
> > x86_64 version also needs it, and calls "get_thread_regcache
> (inferior_thread ())".
> > It would be better to pass the regcache as an argument instead.
> 
> Hi Thiago,
> 
> Do you mean instead or in addition?
> I thought you also need the parameter ("bool &", "shadow_stack_enabled").
> 
> Christina


Hi Thiago,

Since you wrote instead I now have the regcache argument only in the v2 of this series.
https://sourceware.org/pipermail/gdb-patches/2025-February/215650.html
Please let me know if you see any problems with that.

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
Thiago Jung Bauermann Feb. 21, 2025, 5:10 a.m. UTC | #13 
Hello Christina,

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

>> -----Original Message-----
>> From: Schimpe, Christina
>> Sent: Sunday, February 16, 2025 11:45 AM
>> To: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Cc: gdb-patches@sourceware.org
>> Subject: RE: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to get
>> the shadow stack pointer.
>>
>> > -----Original Message-----
>> > From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> > Sent: Saturday, February 15, 2025 4:45 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.
>> >
>> > Hello Christina,
>> >
>> > Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>> >
>> > > "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>> > >
>> > >> 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.
>> >
>> > I was rebasing my Guarded Control Stack code on top of this series,
>> > and noticed that I will need to access the thread's regcache for the
>> > aarch64 implementation of this gdbarch method. Then I noticed that the
>> > x86_64 version also needs it, and calls "get_thread_regcache
>> (inferior_thread ())".
>> > It would be better to pass the regcache as an argument instead.
>>
>> Hi Thiago,
>>
>> Do you mean instead or in addition?
>> I thought you also need the parameter ("bool &", "shadow_stack_enabled").

Sorry for not answering your previous email. I switched my focus to the
variable-length registers thread and forgot that I hadn't answered this
question yet.

> Since you wrote instead I now have the regcache argument only in the v2 of this series.

I wrote "instead" meaning "instead of calling get_thread_regcache inside
of the method implementation". Sorry for being unclear.

> https://sourceware.org/pipermail/gdb-patches/2025-February/215650.html

Strange, for some reason the threading on the patch series got broken up
into three parts. I thought I had missed some of the patches but now
I see I received the whole series.

> Please let me know if you see any problems with that.

For AArch64 GCS I need the shadow_stack_enabled argument so that callers
can tell whether the shadow stack is enabled or disabled.

--
Thiago
diff mbox series

Patch

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index a2ed2a2c011..3aedb3f2600 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1218,6 +1218,14 @@  default_gdbarch_return_value
 				readbuf, writebuf);
 }
 
+/* See arch-utils.h.  */
+
+std::optional<CORE_ADDR>
+default_get_shadow_stack_pointer (gdbarch *gdbarch)
+{
+  return {};
+}
+
 obstack *gdbarch_obstack (gdbarch *arch)
 {
   return &arch->obstack;
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index b59e2ad70c2..471be9c282d 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -325,4 +325,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);
+
 #endif /* GDB_ARCH_UTILS_H */
diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c
index c33c476dfdb..72638f01ad7 100644
--- a/gdb/gdbarch-gen.c
+++ b/gdb/gdbarch-gen.c
@@ -261,6 +261,7 @@  struct gdbarch
   gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
   gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes = default_use_target_description_from_corefile_notes;
   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
@@ -533,6 +534,7 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of read_core_file_mappings, invalid_p == 0.  */
   /* Skip verify of use_target_description_from_corefile_notes, 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 ());
@@ -1404,6 +1406,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);
 }
@@ -5539,3 +5544,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)
+{
+  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);
+}
+
+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 ff14a3666b9..ff511b4a9e0 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1783,6 +1783,8 @@  extern void set_gdbarch_use_target_description_from_corefile_notes (struct gdbar
    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. */
@@ -1792,3 +1794,7 @@  extern bool gdbarch_shadow_stack_push_p (struct gdbarch *gdbarch);
 typedef void (gdbarch_shadow_stack_push_ftype) (struct gdbarch *gdbarch, CORE_ADDR new_addr);
 extern void gdbarch_shadow_stack_push (struct gdbarch *gdbarch, CORE_ADDR new_addr);
 extern void set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch, gdbarch_shadow_stack_push_ftype *shadow_stack_push);
+
+typedef std::optional<CORE_ADDR> (gdbarch_get_shadow_stack_pointer_ftype) (struct gdbarch *gdbarch);
+extern std::optional<CORE_ADDR> gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch);
+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 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,
+)