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

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