[09/12] gdb, gdbarch: Enable inferior calls for shadow stack support.

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

Commit Message

Schimpe, Christina Dec. 20, 2024, 8:04 p.m. UTC
  Inferior calls in GDB reset the current PC to the beginning of the function
that is called.  As no call instruction is executed the new return address
needs to be pushed to the shadow stack and the shadow stack pointer needs
to be updated.

This commit adds a new gdbarch method to push an address on the shadow
stack.  The method is used to adapt the function 'call_function_by_hand_dummy'
for inferior call shadow stack support.
---
 gdb/gdbarch-gen.c         | 32 ++++++++++++++++++++++++++++++++
 gdb/gdbarch-gen.h         | 14 ++++++++++++++
 gdb/gdbarch_components.py | 16 ++++++++++++++++
 gdb/infcall.c             |  6 ++++++
 4 files changed, 68 insertions(+)
  

Comments

Thiago Jung Bauermann Feb. 6, 2025, 3:31 a.m. UTC | #1
"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 6399278c6ae..3a4f1e35a2f 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -1453,6 +1453,12 @@ call_function_by_hand_dummy (struct value *function,
>  				bp_addr, args.size (), args.data (),
>  				sp, return_method, struct_addr);
>
> +  /* Push the return address of the inferior (bp_addr) on the shadow stack
> +     and update the shadow stack pointer.  As we don't execute a call
> +     instruction to start the inferior we need to handle this manually.  */
> +  if (gdbarch_shadow_stack_push_p (gdbarch))
> +    gdbarch_shadow_stack_push (gdbarch, bp_addr);
> +

For AArch64's Guarded Control Stack, instead of adding a new gdbarch
method I added this change to aarch64_push_dummy_call:

  if (aarch64_gcs_is_enabled (regcache))
    aarch64_push_gcs_entry (regcache, bp_addr);

To implement aarch64_gcs_is_enabled I did add a new method to
aarch64_gdbarch_tdep so that OS-independent code in aarch64-tdep.c could call
Linux-specific logic in aarch64-linux-tdep.c:

static bool
aarch64_gcs_is_enabled (regcache *regs)
{
  gdbarch *arch = regs->arch ();
  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (arch);

  if (tdep->gcs_is_enabled == nullptr)
    return false;

  return tdep->gcs_is_enabled (regs);
}

Wouldn't a similar approach work for amd64?

--
Thiago
  
Schimpe, Christina Feb. 6, 2025, 3:07 p.m. UTC | #2
> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Thursday, February 6, 2025 4:32 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 09/12] gdb, gdbarch: Enable inferior calls for shadow stack
> support.
> 
> 
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> 
> > diff --git a/gdb/infcall.c b/gdb/infcall.c index
> > 6399278c6ae..3a4f1e35a2f 100644
> > --- a/gdb/infcall.c
> > +++ b/gdb/infcall.c
> > @@ -1453,6 +1453,12 @@ call_function_by_hand_dummy (struct value
> *function,
> >  				bp_addr, args.size (), args.data (),
> >  				sp, return_method, struct_addr);
> >
> > +  /* Push the return address of the inferior (bp_addr) on the shadow stack
> > +     and update the shadow stack pointer.  As we don't execute a call
> > +     instruction to start the inferior we need to handle this
> > + manually.  */  if (gdbarch_shadow_stack_push_p (gdbarch))
> > +    gdbarch_shadow_stack_push (gdbarch, bp_addr);
> > +
> 
> For AArch64's Guarded Control Stack, instead of adding a new gdbarch method I
> added this change to aarch64_push_dummy_call:
> 
>   if (aarch64_gcs_is_enabled (regcache))
>     aarch64_push_gcs_entry (regcache, bp_addr);
> 
> To implement aarch64_gcs_is_enabled I did add a new method to
> aarch64_gdbarch_tdep so that OS-independent code in aarch64-tdep.c could call
> Linux-specific logic in aarch64-linux-tdep.c:
> 
> static bool
> aarch64_gcs_is_enabled (regcache *regs)
> {
>   gdbarch *arch = regs->arch ();
>   aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (arch);
> 
>   if (tdep->gcs_is_enabled == nullptr)
>     return false;
> 
>   return tdep->gcs_is_enabled (regs);
> }
> 
> Wouldn't a similar approach work for amd64?

Hi Thiago, 

Thank you for the feedback!

I think I could also fix it in amd64_push_dummy_call by implementing it in a similar way than you do. 
But isn't it better to keep the code generic ?

Generic code for handling shadow stacks is also required for the implementation of "bt shadow",
as I tried to provide an implementation that can be used by other architectures as well.

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, 3:57 a.m. UTC | #3
"Schimpe, Christina" <christina.schimpe@intel.com> writes:

>> -----Original Message-----
>> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Sent: Thursday, February 6, 2025 4:32 AM
>> To: Schimpe, Christina <christina.schimpe@intel.com>
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 09/12] gdb, gdbarch: Enable inferior calls for shadow stack
>> support.
>>
>>
>> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>>
>> > diff --git a/gdb/infcall.c b/gdb/infcall.c index
>> > 6399278c6ae..3a4f1e35a2f 100644
>> > --- a/gdb/infcall.c
>> > +++ b/gdb/infcall.c
>> > @@ -1453,6 +1453,12 @@ call_function_by_hand_dummy (struct value
>> *function,
>> >  				bp_addr, args.size (), args.data (),
>> >  				sp, return_method, struct_addr);
>> >
>> > +  /* Push the return address of the inferior (bp_addr) on the shadow stack
>> > +     and update the shadow stack pointer.  As we don't execute a call
>> > +     instruction to start the inferior we need to handle this
>> > + manually.  */  if (gdbarch_shadow_stack_push_p (gdbarch))
>> > +    gdbarch_shadow_stack_push (gdbarch, bp_addr);
>> > +
>>
>> For AArch64's Guarded Control Stack, instead of adding a new gdbarch method I
>> added this change to aarch64_push_dummy_call:
>>
>>   if (aarch64_gcs_is_enabled (regcache))
>>     aarch64_push_gcs_entry (regcache, bp_addr);
>>
>> To implement aarch64_gcs_is_enabled I did add a new method to
>> aarch64_gdbarch_tdep so that OS-independent code in aarch64-tdep.c could call
>> Linux-specific logic in aarch64-linux-tdep.c:
>>
>> static bool
>> aarch64_gcs_is_enabled (regcache *regs)
>> {
>>   gdbarch *arch = regs->arch ();
>>   aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (arch);
>>
>>   if (tdep->gcs_is_enabled == nullptr)
>>     return false;
>>
>>   return tdep->gcs_is_enabled (regs);
>> }
>>
>> Wouldn't a similar approach work for amd64?
>
> Hi Thiago,
>
> Thank you for the feedback!
>
> I think I could also fix it in amd64_push_dummy_call by implementing it in a similar way
> than you do.
> But isn't it better to keep the code generic ?
>
> Generic code for handling shadow stacks is also required for the implementation of "bt
> shadow",
> as I tried to provide an implementation that can be used by other architectures as well.

Yes, that is a good point. I'm on the fence on this one. On the one
hand, there's value in keeping generic code simpler if something can be
handled just as well by arch-specific code, but also it's good to share
code between architectures.

My suggestion for this gdbarch hook then is that it should also accept a
regcache as argument. call_function_by_hand_dummy already obtains it so
that it can call gdbarch_push_dummy_call, so it avoids another call to
inferior_thread deeper in the stack since the hook implementation will
need it.

--
Thiago
  
Schimpe, Christina Feb. 10, 2025, 8:37 a.m. UTC | #4
> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Saturday, February 8, 2025 4:58 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 09/12] gdb, gdbarch: Enable inferior calls for shadow
> stack support.
> 
> 
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> >> Sent: Thursday, February 6, 2025 4:32 AM
> >> To: Schimpe, Christina <christina.schimpe@intel.com>
> >> Cc: gdb-patches@sourceware.org
> >> Subject: Re: [PATCH 09/12] gdb, gdbarch: Enable inferior calls for
> >> shadow stack support.
> >>
> >>
> >> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> >>
> >> > diff --git a/gdb/infcall.c b/gdb/infcall.c index
> >> > 6399278c6ae..3a4f1e35a2f 100644
> >> > --- a/gdb/infcall.c
> >> > +++ b/gdb/infcall.c
> >> > @@ -1453,6 +1453,12 @@ call_function_by_hand_dummy (struct value
> >> *function,
> >> >  				bp_addr, args.size (), args.data (),
> >> >  				sp, return_method, struct_addr);
> >> >
> >> > +  /* Push the return address of the inferior (bp_addr) on the shadow
> stack
> >> > +     and update the shadow stack pointer.  As we don't execute a call
> >> > +     instruction to start the inferior we need to handle this
> >> > + manually.  */  if (gdbarch_shadow_stack_push_p (gdbarch))
> >> > +    gdbarch_shadow_stack_push (gdbarch, bp_addr);
> >> > +
> >>
> >> For AArch64's Guarded Control Stack, instead of adding a new gdbarch
> >> method I added this change to aarch64_push_dummy_call:
> >>
> >>   if (aarch64_gcs_is_enabled (regcache))
> >>     aarch64_push_gcs_entry (regcache, bp_addr);
> >>
> >> To implement aarch64_gcs_is_enabled I did add a new method to
> >> aarch64_gdbarch_tdep so that OS-independent code in aarch64-tdep.c
> >> could call Linux-specific logic in aarch64-linux-tdep.c:
> >>
> >> static bool
> >> aarch64_gcs_is_enabled (regcache *regs) {
> >>   gdbarch *arch = regs->arch ();
> >>   aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep>
> >> (arch);
> >>
> >>   if (tdep->gcs_is_enabled == nullptr)
> >>     return false;
> >>
> >>   return tdep->gcs_is_enabled (regs); }
> >>
> >> Wouldn't a similar approach work for amd64?
> >
> > Hi Thiago,
> >
> > Thank you for the feedback!
> >
> > I think I could also fix it in amd64_push_dummy_call by implementing
> > it in a similar way than you do.
> > But isn't it better to keep the code generic ?
> >
> > Generic code for handling shadow stacks is also required for the
> > implementation of "bt shadow", as I tried to provide an implementation
> > that can be used by other architectures as well.
> 
> Yes, that is a good point. I'm on the fence on this one. On the one hand,
> there's value in keeping generic code simpler if something can be handled just
> as well by arch-specific code, but also it's good to share code between
> architectures.

Ok, let's hope for more feedback on this open. 

> My suggestion for this gdbarch hook then is that it should also accept a
> regcache as argument. call_function_by_hand_dummy already obtains it so
> that it can call gdbarch_push_dummy_call, so it avoids another call to
> inferior_thread deeper in the stack since the hook implementation will need it.

Good point. This shouldn't be a problem. 

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
  

Patch

diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c
index d05c7a3cbdf..c33c476dfdb 100644
--- a/gdb/gdbarch-gen.c
+++ b/gdb/gdbarch-gen.c
@@ -260,6 +260,7 @@  struct gdbarch
   gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
   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;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -531,6 +532,7 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of get_pc_address_flags, invalid_p == 0.  */
   /* 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.  */
   if (!log.empty ())
     internal_error (_("verify_gdbarch: the following are invalid ...%s"),
 		    log.c_str ());
@@ -1396,6 +1398,12 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
 	      "gdbarch_dump: use_target_description_from_corefile_notes = <%s>\n",
 	      host_address_to_string (gdbarch->use_target_description_from_corefile_notes));
+  gdb_printf (file,
+	      "gdbarch_dump: gdbarch_shadow_stack_push_p() = %d\n",
+	      gdbarch_shadow_stack_push_p (gdbarch));
+  gdb_printf (file,
+	      "gdbarch_dump: shadow_stack_push = <%s>\n",
+	      host_address_to_string (gdbarch->shadow_stack_push));
   if (gdbarch->dump_tdep != NULL)
     gdbarch->dump_tdep (gdbarch, file);
 }
@@ -5507,3 +5515,27 @@  set_gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch,
 {
   gdbarch->use_target_description_from_corefile_notes = use_target_description_from_corefile_notes;
 }
+
+bool
+gdbarch_shadow_stack_push_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->shadow_stack_push != NULL;
+}
+
+void
+gdbarch_shadow_stack_push (struct gdbarch *gdbarch, CORE_ADDR new_addr)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->shadow_stack_push != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_shadow_stack_push called\n");
+  gdbarch->shadow_stack_push (gdbarch, new_addr);
+}
+
+void
+set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch,
+			       gdbarch_shadow_stack_push_ftype shadow_stack_push)
+{
+  gdbarch->shadow_stack_push = shadow_stack_push;
+}
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index 9fda85f860f..ff14a3666b9 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1778,3 +1778,17 @@  extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarc
 typedef bool (gdbarch_use_target_description_from_corefile_notes_ftype) (struct gdbarch *gdbarch, struct bfd *corefile_bfd);
 extern bool gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, struct bfd *corefile_bfd);
 extern void set_gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes);
+
+/* 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.
+
+   Push the address NEW_ADDR on the shadow stack and update the shadow stack
+   pointer. */
+
+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);
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index cc7c6d8677b..52f265e8e0e 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -2815,3 +2815,19 @@  The corefile's bfd is passed through COREFILE_BFD.
     predefault="default_use_target_description_from_corefile_notes",
     invalid=False,
 )
+
+Method(
+    comment="""
+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.
+
+Push the address NEW_ADDR on the shadow stack and update the shadow stack
+pointer.
+""",
+    type="void",
+    name="shadow_stack_push",
+    params=[("CORE_ADDR", "new_addr")],
+    predicate=True,
+)
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 6399278c6ae..3a4f1e35a2f 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -1453,6 +1453,12 @@  call_function_by_hand_dummy (struct value *function,
 				bp_addr, args.size (), args.data (),
 				sp, return_method, struct_addr);
 
+  /* Push the return address of the inferior (bp_addr) on the shadow stack
+     and update the shadow stack pointer.  As we don't execute a call
+     instruction to start the inferior we need to handle this manually.  */
+  if (gdbarch_shadow_stack_push_p (gdbarch))
+    gdbarch_shadow_stack_push (gdbarch, bp_addr);
+
   /* Set up a frame ID for the dummy frame so we can pass it to
      set_momentary_breakpoint.  We need to give the breakpoint a frame
      ID so that the breakpoint code can correctly re-identify the