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

Schimpe, Christina
2025-08-01T12:55:05Z

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