Re: [PATCH 06/12] gdb, gdbserver: Add support of Intel shadow stack pointer register.

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

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

> Hi Thiago,
>
> Thank you for the review. Please see my comments to your feedback below.
>
>> -----Original Message-----
>> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Sent: Thursday, February 6, 2025 4:14 AM
>> To: Schimpe, Christina <christina.schimpe@intel.com>
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 06/12] gdb, gdbserver: Add support of Intel shadow stack
>> pointer register.
>>
>>
>> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>>
>> > diff --git a/gdb/arch/i386.c b/gdb/arch/i386.c index
>> > 4a39028a472..59daaa4c583 100644
>> > --- a/gdb/arch/i386.c
>> > +++ b/gdb/arch/i386.c
>> > @@ -28,6 +28,7 @@
>> >  #include "../features/i386/32bit-avx512.c"
>> >  #include "../features/i386/32bit-segments.c"
>> >  #include "../features/i386/pkeys.c"
>> > +#include "../features/i386/32bit-ssp.c"
>> >
>> >  /* See i386.h.  */
>> >
>> > @@ -66,5 +67,8 @@ i386_create_target_description (uint64_t
>> xstate_bv_mask, bool is_linux,
>> >    if (xstate_bv_mask & X86_XSTATE_PKRU)
>> >      regnum = create_feature_i386_pkeys (tdesc.get (), regnum);
>> >
>> > +  if (xstate_bv_mask & X86_XSTATE_CET_U)
>> > +    regnum = create_feature_i386_32bit_ssp (tdesc.get (), regnum);
>> > +
>> >    return tdesc.release ();
>> >  }
>>
>> The patch description mentions that "32 bit support is not covered due to missing
>> linux kernel support". Is this change useful, or is it unreachable code?
>
> I think I consistently did not implement 32 bit linux support, but added the code
> for 32 bit support in locations which are not linux dependent, as preparation for
> other OS. But for now yes, this should be unreachable code.
> Should I (1) Remove the code or (2) improve the commit message ?
> I'd be in favour of (2),  would that be acceptable?

In the general case, I lean towards (1), but in this case it's just two
simple lines, and also in an arch-specific corner of GDB so it's not
really a problem. It's certainly acceptable in my view.

>> > diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c index
>> > d1fece717a7..5bbd4640e30 100644
>> > --- a/gdb/x86-linux-nat.c
>> > +++ b/gdb/x86-linux-nat.c
>> > @@ -41,6 +41,7 @@
>> >  #include "nat/x86-linux.h"
>> >  #include "nat/x86-linux-dregs.h"
>> >  #include "nat/linux-ptrace.h"
>> > +#include "x86-tdep.h"
>> >  #include "nat/x86-linux-tdesc.h"
>> >
>> >  /* linux_nat_target::low_new_fork implementation.  */ @@ -97,11
>> > +98,10 @@ const struct target_desc *
>> > x86_linux_nat_target::read_description ()  {
>> >    /* The x86_linux_tdesc_for_tid call only reads xcr0 the first time it is
>> > -     called.  The mask is stored in XSTATE_BV_STORAGE and reused on
>> > -     subsequent calls.  Note that GDB currently supports features for user
>> > -     state components only.  However, once supervisor state components are
>> > -     supported in GDB XSTATE_BV_STORAGE will not be configured based on
>> > -     xcr0 only.  */
>> > +     called.  Also it checks the enablement state of features which are
>> > +     not configured in xcr0, such as CET shadow stack.  Once the
>> > + supported
>>
>> The "not" above should be removed.
>
> You mean the "not" in this sentence?
> "Also it checks the enablement state of features which are not configured in xcr0, such as
> CET shadow stack.  "
>
> This should be correct in that context. CET shadow stack is not configured in xcr0.

Ah, I see. Sorry, I had misread the comment.

>> > +     features are identified, the XSTATE_BV_STORAGE value is configured
>> > +     accordingly and preserved for subsequent calls of this function.
>> > + */
>> >    static uint64_t xstate_bv_storage;
>> >
>> >    if (inferior_ptid == null_ptid)
>> > @@ -215,6 +215,46 @@ x86_linux_get_thread_area (pid_t pid, void *addr,
>> > unsigned int *base_addr)  }
>> >
>>
>>
>> >
>> > +/* See x86-linux-nat.h.  */
>> > +
>> > +void
>> > +x86_linux_fetch_ssp (regcache *regcache, const int tid) {
>> > +  uint64_t ssp = 0x0;
>> > +  iovec iov {&ssp, sizeof (ssp)};
>> > +
>> > +  /* The shadow stack may be enabled and disabled at runtime.  Reading the
>> > +     ssp might fail as shadow stack was not activated for the current
>> > +     thread.  We don't want to show a warning but silently return.  The
>> > +     register will be shown as unavailable for the user.  */  if
>> > + (ptrace (PTRACE_GETREGSET, tid, NT_X86_SHSTK, &iov) != 0)
>> > +    return;
>>
>> In case the ptrace fails and there is already an old value for ssp in regcache,
>> shouldn't it be removed from it?
>
> Hm, it doesn't seem to be a problem. I ran a test using inline enablement of shadow stack:
> ~~~
> (gdb) p $pl3_ssp
> $1 = (void *) 0x7ffff7c00000
> (gdb) n
> 54	   if (ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK)) {
> (gdb) n
> 58	   return ret;
> (gdb) p $pl3_ssp
> $2 = <unavailable>
> ~~~
> So it should be fine from my perspective. Or do you see another potential issue?

Each time the inferior continues, GDB drops the existing regcaches and
builds new ones, so the values from $1 and $2 in the output above come
from different regcache instances.

I made the comment thinking that if GDB knows that ssp is unavailable
then it should strive to make sure that is reflected in the
regcache. But come to think of it, if the inferior is stopped then
whether ssp is available or not won't change until it runs again and the
scenario I was considering can't happen. Sorry about the false alarm.

--
Thiago