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

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

Hi Andrew,

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Friday, July 25, 2025 5:04 PM
> To: Andrew Burgess <aburgess@redhat.com>; gdb-patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com
> Subject: RE: [PATCH v5 06/12] gdb, gdbserver: Add support of Intel shadow
> stack pointer register.
> 
> Hi Andrew,
> 
> Thanks a lot for the review! I have some questions for your feedback, please
> find my comments below.
> 
> > -----Original Message-----
> > From: Andrew Burgess <aburgess@redhat.com>
> > Sent: Friday, July 25, 2025 2:50 PM
> > To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> > patches@sourceware.org
> > Cc: thiago.bauermann@linaro.org; luis.machado@arm.com
> > Subject: Re: [PATCH v5 06/12] gdb, gdbserver: Add support of Intel
> > shadow stack pointer register.
> >
> > Christina Schimpe <christina.schimpe@intel.com> writes:
> >
> > > This patch adds the user mode register PL3_SSP which is part of the
> > > Intel(R) Control-Flow Enforcement Technology (CET) feature for
> > > support of shadow stack.
> > > For now, only native and remote debugging support for shadow stack
> > > userspace on amd64 linux are covered by this patch including 64 bit
> > > and
> > > x32 support.  32 bit support is not covered due to missing Linux
> > > kernel support.
> > >
> > > This patch requires fixing the test
> > > gdb.base/inline-frame-cycle-unwind
> > > which is failing in case the shadow stack pointer is unavailable.
> > > Such a state is possible if shadow stack is disabled for the current
> > > thread but supported by HW.
> > >
> > > This test uses the Python unwinder inline-frame-cycle-unwind.py
> > > which fakes the cyclic stack cycle by reading the pending frame's
> > > registers and adding them to the unwinder:
> > >
> > > ~~~
> > > for reg in pending_frame.architecture().registers("general"):
> > >      val = pending_frame.read_register(reg)
> > >      unwinder.add_saved_register(reg, val)
> > >      return unwinder
> > > ~~~
> > >
> > > However, in case the python unwinder is used we add a register
> > > (pl3_ssp) that is unavailable.  This leads to a NOT_AVAILABLE_ERROR
> > > caught in gdb/frame-unwind.c:frame_unwind_try_unwinder and it is
> > > continued with standard unwinders.  This destroys the faked cyclic
> > > behavior and the stack is further unwinded after frame 5.
> > >
> > > In the working scenario an error should be triggered:
> > > ~~~
> > > bt
> > > 0  inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:49^M
> > > 1  normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
> > > 2  0x000055555555516e in inline_func () at
> > > /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M
> > > 3  normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
> > > 4  0x000055555555516e in inline_func () at
> > > /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M
> > > 5  normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
> > > Backtrace stopped: previous frame identical to this frame (corrupt
> > > stack?)
> > > (gdb) PASS: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 5:
> > > backtrace when the unwind is broken at frame 5 ~~~
> > >
> > > To fix the Python unwinder, we simply skip the unavailable registers.
> > >
> > > Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> > > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> > > Reviewed-By: Luis Machado <luis.machado@arm.com>
> > > ---
> > >  gdb/NEWS                                      |  3 +
> > >  gdb/amd64-linux-nat.c                         | 17 +++++
> > >  gdb/amd64-linux-tdep.c                        |  1 +
> > >  gdb/amd64-tdep.c                              |  6 +-
> > >  gdb/amd64-tdep.h                              |  1 +
> > >  gdb/arch/amd64.c                              | 10 +++
> > >  gdb/arch/i386.c                               |  4 ++
> > >  gdb/arch/x86-linux-tdesc-features.c           |  1 +
> > >  gdb/doc/gdb.texinfo                           |  4 ++
> > >  gdb/features/Makefile                         |  2 +
> > >  gdb/features/i386/32bit-ssp.c                 | 14 ++++
> > >  gdb/features/i386/32bit-ssp.xml               | 11 +++
> > >  gdb/features/i386/64bit-ssp.c                 | 14 ++++
> > >  gdb/features/i386/64bit-ssp.xml               | 11 +++
> > >  gdb/i386-tdep.c                               | 22 +++++-
> > >  gdb/i386-tdep.h                               |  4 ++
> > >  gdb/nat/x86-linux-tdesc.c                     |  2 +
> > >  gdb/nat/x86-linux.c                           | 57 +++++++++++++++
> > >  gdb/nat/x86-linux.h                           |  4 ++
> > >  gdb/testsuite/gdb.arch/amd64-shadow-stack.c   | 22 ++++++
> > >  gdb/testsuite/gdb.arch/amd64-ssp.exp          | 50 +++++++++++++
> > >  .../gdb.base/inline-frame-cycle-unwind.py     |  4 ++
> > >  gdb/testsuite/lib/gdb.exp                     | 70 +++++++++++++++++++
> > >  gdb/x86-linux-nat.c                           | 49 +++++++++++--
> > >  gdb/x86-linux-nat.h                           | 11 +++
> > >  gdb/x86-tdep.c                                | 21 ++++++
> > >  gdb/x86-tdep.h                                |  9 +++
> > >  gdbserver/linux-x86-low.cc                    | 28 +++++++-
> > >  gdbsupport/x86-xstate.h                       |  5 +-
> > >  29 files changed, 447 insertions(+), 10 deletions(-)  create mode
> > > 100644 gdb/features/i386/32bit-ssp.c  create mode 100644
> > > gdb/features/i386/32bit-ssp.xml  create mode 100644
> > > gdb/features/i386/64bit-ssp.c  create mode 100644
> > > gdb/features/i386/64bit-ssp.xml  create mode 100644
> > > gdb/testsuite/gdb.arch/amd64-shadow-stack.c
> > >  create mode 100644 gdb/testsuite/gdb.arch/amd64-ssp.exp
> > >
> >
> >
> > > diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c index
> > > dbb9b3223cb..4df99ccca54 100644
> > > --- a/gdb/amd64-linux-nat.c
> > > +++ b/gdb/amd64-linux-nat.c
> > > @@ -32,6 +32,7 @@
> > >  #include "amd64-tdep.h"
> > >  #include "amd64-linux-tdep.h"
> > >  #include "i386-linux-tdep.h"
> > > +#include "x86-tdep.h"
> > >  #include "gdbsupport/x86-xstate.h"
> > >
> > >  #include "x86-linux-nat.h"
> > > @@ -237,6 +238,14 @@ amd64_linux_nat_target::fetch_registers
> (struct
> > > regcache *regcache, int regnum)
> > >
> > >        if (have_ptrace_getregset == TRIBOOL_TRUE)
> > >  	{
> > > +	  if ((regnum == -1 && tdep->ssp_regnum > 0)
> > > +	      || (regnum != -1 && regnum == tdep->ssp_regnum))
> >
> > It's really nit-picking, but I don't think the '>' here check is
> > correct.  You're checking that tdep->ssp_regnum has been assigned a
> > value, right?  And it's default value is -1, so, shouldn't the check really be
> '>= 0' or '!= -1' maybe?
> >
> > The same pattern is repeated below in ::store_registers.
> >
> > Ahh, reading further on, I see that (not just you), but all the
> > *_regnum fields in i386_gdbarch_tdep start set to 0 (which is a valid
> > register number), but are set to -1 if the feature is not found.
> > Maybe I'm missing something incredibly clever about this ... but I
> > suspect this code is just showing its age a little.  I still think the validity
> check should be against -1.
> 
> Yes I agree, it seems to me that there is something inconsistent in GDB.
> 
> The current default value is 0, we set all regnums to 0 initially in gdb/i386-
> tdep.h And we set it to -1 to indicate the absence of that specific register.
> 
> But on the other hand the enums (enum amd64_regnum/enum
> i386_regnum) defining the register numbers start at 0.
> 
> So that seems to be a separate issue one could consider to fix.
> Maybe the default should be simply -1, instead of 0 ?
> 
> But for my specific patch here the best would be to compare against against
> -1, I agree and will fix.

I created this patch to address the regnum defaults as suggested above:

https://sourceware.org/pipermail/gdb-patches/2025-August/219649.html

Kind Regards,
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