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

["Schimpe, Christina" <christina.schimpe@intel.com>]

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