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

[Andrew Burgess <aburgess@redhat.com>]

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

> Hi Andrew,
>
>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Tuesday, August 5, 2025 3:57 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.
>> 
>> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>> 
>> > 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.
>> >
>> >> > +	    {
>> >> > +	      x86_linux_fetch_ssp (regcache, tid);
>> >> > +	      if (regnum != -1)
>> >> > +		return;
>> >> > +	    }
>> >> > +
>> >> >  	  /* Pre-4.14 kernels have a bug (fixed by commit 0852b374173b
>> >> >  	     "x86/fpu: Add FPU state copying quirk to handle XRSTOR failure
>> on
>> >> >  	     Intel Skylake CPUs") that sometimes causes the mxcsr
>> >> > location in @@ -302,6 +311,14 @@
>> >> > amd64_linux_nat_target::store_registers (struct
>> >> regcache *regcache, int regnum)
>> >> >        if (have_ptrace_getregset == TRIBOOL_TRUE)
>> >> >  	{
>> >> >  	  gdb::byte_vector xstateregs (tdep->xsave_layout.sizeof_xsave);
>> >> > +	  if ((regnum == -1 && tdep->ssp_regnum > 0)
>> >> > +	      || (regnum != -1 && regnum == tdep->ssp_regnum))
>> >> > +	    {
>> >> > +	      x86_linux_store_ssp (regcache, tid);
>> >> > +	      if (regnum != -1)
>> >> > +		return;
>> >> > +	    }
>> >> > +
>> >> >  	  struct iovec iov;
>> >> >
>> >> >  	  iov.iov_base = xstateregs.data ();
>> >>
>> >>
>> >> > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index
>> >> > 4ef640698bd..0881ac4aee5 100644
>> >> > --- a/gdb/doc/gdb.texinfo
>> >> > +++ b/gdb/doc/gdb.texinfo
>> >> > @@ -49959,6 +49959,10 @@ The @samp{org.gnu.gdb.i386.pkeys}
>> feature
>> >> is
>> >> > optional.  It should  describe a single register, @samp{pkru}.  It
>> >> > is a 32-bit register  valid for i386 and amd64.
>> >> >
>> >> > +The @samp{org.gnu.gdb.i386.pl3_ssp} feature is optional.  It
>> >> > +should describe the user mode register @samp{pl3_ssp} which has 64
>> >> > +bits on amd64.  Following the restriction of the Linux kernel,
>> >> > +only amd64 is
>> >> supported for now.
>> >>
>> >> You mention a couple of times in this patch that the shadow stack is
>> >> only supported (for now) on amd64 (& x32), but you have added an i386
>> >> version of the org.gnu.gdb.i386.pl3_ssp feature with a 32-bit pl3_ssp
>> register.
>> >
>> > We need the 32-bit pl3_ssp register for x32, since for x32 the shadow
>> stack pointer has 32 bits only.
>> >
>> >> This feature is (as far as I can tell) instantiated for 32-bit
>> >> targets, which means gdbserver will send out target descriptions
>> containing this feature.
>> >>
>> >> I think either (a) you should drop the i386 xml file and feature, or
>> >> (b) my preferred choice, document the i386 version here.  It's still
>> >> fine to say that the 32-bit (i386) register is ignored by GDB for now
>> >> though.  I had a go at rewording the paragraph, but I'm not 100% sure its
>> OK yet:
>> >>
>> >>   The @samp{org.gnu.gdb.i386.pl3_ssp} feature is optional.  It should
>> >>   describe the user mode register @samp{pl3_ssp} which has 64 bits on
>> >>   amd64 and 32 bits on i386.  Following the restriction of the Linux
>> >>   kernel, only GDB for amd64 targets makes use of this feature for now.
>> >
>> > I missed to describe x32 here, actually. So I'd write it as follows:
>> >
>> > The @samp{org.gnu.gdb.i386.pl3_ssp} feature is optional.  It should
>> > describe the user mode register @samp{pl3_ssp} which has 64 bits on
>> > amd64, 32 bits on amd64 with  32-bit pointer size (X32) and 32 bits on
>> i386.
>> > Following the restriction of the Linux kernel, only GDB for amd64
>> > targets makes use of this feature for now.
>> >
>> > What do you think?
>> 
>> Sounds great.
>> 
>> >
>> >>
>> >> > +
>> >> >  @node LoongArch Features
>> >> >  @subsection LoongArch Features
>> >> >  @cindex target descriptions, LoongArch Features diff --git
>> >> > a/gdb/features/Makefile b/gdb/features/Makefile index
>> >> > 7a8c7999733..2afda1ccd00 100644
>> >> > --- a/gdb/features/Makefile
>> >> > +++ b/gdb/features/Makefile
>> >> > @@ -225,6 +225,7 @@ FEATURE_XMLFILES = aarch64-core.xml \
>> >> >  	i386/32bit-avx.xml \
>> >> >  	i386/32bit-avx512.xml \
>> >> >  	i386/32bit-segments.xml \
>> >> > +	i386/32bit-ssp.xml \
>> >> >  	i386/64bit-avx512.xml \
>> >> >  	i386/64bit-core.xml \
>> >> >  	i386/64bit-segments.xml \
>> >> > @@ -232,6 +233,7 @@ FEATURE_XMLFILES = aarch64-core.xml \
>> >> >  	i386/64bit-linux.xml \
>> >> >  	i386/64bit-sse.xml \
>> >> >  	i386/pkeys.xml \
>> >> > +	i386/64bit-ssp.xml \
>> >> >  	i386/x32-core.xml \
>> >> >  	loongarch/base32.xml \
>> >> >  	loongarch/base64.xml \
>> >> > diff --git a/gdb/features/i386/32bit-ssp.c
>> >> > b/gdb/features/i386/32bit-ssp.c new file mode 100644 index
>> >> > 00000000000..991bae3c1e6
>> >> > --- /dev/null
>> >> > +++ b/gdb/features/i386/32bit-ssp.c
>> >> > @@ -0,0 +1,14 @@
>> >> > +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
>> >> > +  Original: 32bit-ssp.xml */
>> >> > +
>> >> > +#include "gdbsupport/tdesc.h"
>> >> > +
>> >> > +static int
>> >> > +create_feature_i386_32bit_ssp (struct target_desc *result, long
>> >> > +regnum) {
>> >> > +  struct tdesc_feature *feature;
>> >> > +
>> >> > +  feature = tdesc_create_feature (result,
>> >> > +"org.gnu.gdb.i386.pl3_ssp");
>> >> > +  tdesc_create_reg (feature, "pl3_ssp", regnum++, 1, NULL, 32,
>> >> > +"data_ptr");
>> >> > +  return regnum;
>> >> > +}
>> >> > diff --git a/gdb/features/i386/32bit-ssp.xml
>> >> > b/gdb/features/i386/32bit-ssp.xml new file mode 100644 index
>> >> > 00000000000..d17e7004eec
>> >> > --- /dev/null
>> >> > +++ b/gdb/features/i386/32bit-ssp.xml
>> >> > @@ -0,0 +1,11 @@
>> >> > +<?xml version="1.0"?>
>> >> > +<!-- Copyright (C) 2022-2024 Free Software Foundation, Inc.
>> >> > +
>> >> > +     Copying and distribution of this file, with or without modification,
>> >> > +     are permitted in any medium without royalty provided the
>> copyright
>> >> > +     notice and this notice are preserved.  -->
>> >> > +
>> >> > +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature
>> >> > +name="org.gnu.gdb.i386.pl3_ssp">
>> >> > +  <reg name="pl3_ssp" bitsize="32" type="data_ptr"/> </feature>
>> >> > diff --git a/gdb/features/i386/64bit-ssp.c
>> >> > b/gdb/features/i386/64bit-ssp.c new file mode 100644 index
>> >> > 00000000000..5468099ddf6
>> >> > --- /dev/null
>> >> > +++ b/gdb/features/i386/64bit-ssp.c
>> >> > @@ -0,0 +1,14 @@
>> >> > +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
>> >> > +  Original: 64bit-ssp.xml */
>> >> > +
>> >> > +#include "gdbsupport/tdesc.h"
>> >> > +
>> >> > +static int
>> >> > +create_feature_i386_64bit_ssp (struct target_desc *result, long
>> >> > +regnum) {
>> >> > +  struct tdesc_feature *feature;
>> >> > +
>> >> > +  feature = tdesc_create_feature (result,
>> >> > +"org.gnu.gdb.i386.pl3_ssp");
>> >> > +  tdesc_create_reg (feature, "pl3_ssp", regnum++, 1, NULL, 64,
>> >> > +"data_ptr");
>> >> > +  return regnum;
>> >> > +}
>> >> > diff --git a/gdb/features/i386/64bit-ssp.xml
>> >> > b/gdb/features/i386/64bit-ssp.xml new file mode 100644 index
>> >> > 00000000000..a0688d018a5
>> >> > --- /dev/null
>> >> > +++ b/gdb/features/i386/64bit-ssp.xml
>> >> > @@ -0,0 +1,11 @@
>> >> > +<?xml version="1.0"?>
>> >> > +<!-- Copyright (C) 2022-2024 Free Software Foundation, Inc.
>> >> > +
>> >> > +     Copying and distribution of this file, with or without modification,
>> >> > +     are permitted in any medium without royalty provided the
>> copyright
>> >> > +     notice and this notice are preserved.  -->
>> >> > +
>> >> > +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature
>> >> > +name="org.gnu.gdb.i386.pl3_ssp">
>> >> > +  <reg name="pl3_ssp" bitsize="64" type="data_ptr"/> </feature>
>> >> > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index
>> >> > 90ff0c5c706..8eb5b4fac86 100644
>> >> > --- a/gdb/i386-tdep.c
>> >> > +++ b/gdb/i386-tdep.c
>> >> > @@ -8403,7 +8403,8 @@ i386_validate_tdesc_p (i386_gdbarch_tdep
>> *tdep,
>> >> >    const struct tdesc_feature *feature_core;
>> >> >
>> >> >    const struct tdesc_feature *feature_sse, *feature_avx,
>> *feature_avx512,
>> >> > -			     *feature_pkeys, *feature_segments;
>> >> > +			     *feature_pkeys, *feature_segments,
>> >> > +			     *feature_pl3_ssp;
>> >> >    int i, num_regs, valid_p;
>> >> >
>> >> >    if (! tdesc_has_registers (tdesc)) @@ -8429,6 +8430,9 @@
>> >> > i386_validate_tdesc_p (i386_gdbarch_tdep *tdep,
>> >> >    /* Try PKEYS  */
>> >> >    feature_pkeys = tdesc_find_feature (tdesc,
>> >> > "org.gnu.gdb.i386.pkeys");
>> >> >
>> >> > +  /* Try Shadow Stack.  */
>> >> > +  feature_pl3_ssp = tdesc_find_feature (tdesc,
>> >> > + "org.gnu.gdb.i386.pl3_ssp");
>> >> > +
>> >> >    valid_p = 1;
>> >> >
>> >> >    /* The XCR0 bits.  */
>> >> > @@ -8544,6 +8548,15 @@ i386_validate_tdesc_p (i386_gdbarch_tdep
>> >> *tdep,
>> >> >  					    tdep->pkeys_register_names[i]);
>> >> >      }
>> >> >
>> >> > +  if (feature_pl3_ssp != nullptr)
>> >> > +    {
>> >> > +      if (tdep->ssp_regnum < 0)
>> >> > +	tdep->ssp_regnum = I386_PL3_SSP_REGNUM;
>> >> > +
>> >> > +      valid_p &= tdesc_numbered_register (feature_pl3_ssp, tdesc_data,
>> >> > +					  tdep->ssp_regnum, "pl3_ssp");
>> >> > +    }
>> >> > +
>> >> >    return valid_p;
>> >> >  }
>> >> >
>> >> > @@ -8835,6 +8848,9 @@ i386_gdbarch_init (struct gdbarch_info info,
>> >> struct gdbarch_list *arches)
>> >> >    /* No segment base registers.  */
>> >> >    tdep->fsbase_regnum = -1;
>> >> >
>> >> > +  /* No shadow stack pointer register.  */  tdep->ssp_regnum = -1;
>> >> > +
>> >> >    tdesc_arch_data_up tdesc_data = tdesc_data_alloc ();
>> >> >
>> >> >    set_gdbarch_relocate_instruction (gdbarch,
>> >> > i386_relocate_instruction); @@ -8955,13 +8971,15 @@ const struct
>> >> > target_desc *  i386_target_description (uint64_t xstate_bv_mask,
>> >> > bool
>> >> > segments)  {
>> >> >    static target_desc *i386_tdescs \
>> >> > -    [2/*SSE*/][2/*AVX*/][2/*AVX512*/][2/*PKRU*/][2/*segments*/] =
>> {};
>> >> > +    [2/*SSE*/][2/*AVX*/][2/*AVX512*/][2/*PKRU*/][2/*CET_U*/] \
>> >> > +    [2/*segments*/] = {};
>> >> >    target_desc **tdesc;
>> >> >
>> >> >    tdesc = &i386_tdescs[(xstate_bv_mask & X86_XSTATE_SSE) ? 1 : 0]
>> >> >      [(xstate_bv_mask & X86_XSTATE_AVX) ? 1 : 0]
>> >> >      [(xstate_bv_mask & X86_XSTATE_AVX512) ? 1 : 0]
>> >> >      [(xstate_bv_mask & X86_XSTATE_PKRU) ? 1 : 0]
>> >> > +    [(xstate_bv_mask & X86_XSTATE_CET_U) ? 1 : 0]
>> >> >      [segments ? 1 : 0];
>> >> >
>> >> >    if (*tdesc == NULL)
>> >> > diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h index
>> >> > 239bc8674e8..60d6f3eb732 100644
>> >> > --- a/gdb/i386-tdep.h
>> >> > +++ b/gdb/i386-tdep.h
>> >> > @@ -191,6 +191,9 @@ struct i386_gdbarch_tdep : gdbarch_tdep_base
>> >> >    /* PKEYS register names.  */
>> >> >    const char * const *pkeys_register_names = nullptr;
>> >> >
>> >> > +  /* Shadow stack pointer register.  */  int ssp_regnum = 0;
>> >>
>> >> I know all the other *_regnum fields start as 0, but I really don't
>> >> understand why.  Surely starting as -1 would make more sense?  This
>> >> isn't a hard requirement, but maybe you see some reason why these
>> >> fields should start at 0 that I'm missing?
>> >
>> > I agree. As stated before setting the regnums to -1 here by default
>> > would make more sense to me. So I think I should set ssp_regnum here to
>> -1.
>> > I'd do that, unless I see some issues with that when testing of course.
>> >
>> > The other registers set to 0 here could be fixed separately.
>> >
>> > Does that sound reasonable?
>> 
>> Yes absolutely, I certainly don't want you to change unrelated code as part
>> of this work, but we might as well improve new code as its added.
>> Your proposal sounds perfect to me.
>
> The patch was pretty straight-forward. 😊
> Thanks for providing the feedback here, I think this makes the code much better.
>
>> >
>> >> > +
>> >> >    /* Register number for %fsbase.  Set this to -1 to indicate the
>> >> >       absence of segment base registers.  */
>> >> >    int fsbase_regnum = 0;
>> >> > @@ -293,6 +296,7 @@ enum i386_regnum
>> >> >    I386_ZMM0H_REGNUM,		/* %zmm0h */
>> >> >    I386_ZMM7H_REGNUM = I386_ZMM0H_REGNUM + 7,
>> >> >    I386_PKRU_REGNUM,
>> >> > +  I386_PL3_SSP_REGNUM,
>> >> >    I386_FSBASE_REGNUM,
>> >> >    I386_GSBASE_REGNUM
>> >> >  };
>> >>
>> >>
>> >> > diff --git a/gdb/testsuite/gdb.arch/amd64-ssp.exp
>> >> > b/gdb/testsuite/gdb.arch/amd64-ssp.exp
>> >> > new file mode 100644
>> >> > index 00000000000..6ddc875b9a3
>> >> > --- /dev/null
>> >> > +++ b/gdb/testsuite/gdb.arch/amd64-ssp.exp
>> >> > @@ -0,0 +1,50 @@
>> >> > +# Copyright 2018-2024 Free Software Foundation, Inc.
>> >> > +
>> >> > +# This program is free software; you can redistribute it and/or
>> >> > +modify # it under the terms of the GNU General Public License as
>> >> > +published by # the Free Software Foundation; either version 3 of
>> >> > +the License, or # (at your option) any later version.
>> >> > +#
>> >> > +# This program is distributed in the hope that it will be useful,
>> >> > +# but WITHOUT ANY WARRANTY; without even the implied warranty
>> of #
>> >> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the #
>> >> GNU
>> >> > +General Public License for more details.
>> >> > +#
>> >> > +# You should have received a copy of the GNU General Public
>> >> > +License # along with this program.  If not, see
>> <http://www.gnu.org/licenses/>.
>> >> > +
>> >> > +# Test accessing the shadow stack pointer register.
>> >> > +
>> >> > +require allow_ssp_tests
>> >> > +
>> >> > +standard_testfile amd64-shadow-stack.c
>> >>
>> >> Test source files should be named to match the .exp file.  But in
>> >> this case 'amd64-shadow-stack' seems better than 'amd64-ssp', so I'd
>> >> renamed the .exp file to amd64-shadow-stack.exp, then this line can be
>> just:
>> >>
>> >>   standard_testfile
>> >
>> > Yes, I agree and will fix.
>> >
>> >> > +
>> >> > +save_vars { ::env(GLIBC_TUNABLES) } {
>> >> > +
>> >> > +    append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
>> >> > +
>> >> > +    if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
>> >> > +	  additional_flags="-fcf-protection=return"] } {
>> >> > +	return -1
>> >> > +    }
>> >> > +
>> >> > +    if {![runto_main]} {
>> >> > +	return -1
>> >> > +    }
>> >>
>> >> The 'return -1' can become just 'return'.  The return value doesn't
>> >> mean anything.
>> >
>> > True will fix (also in following patches, that have the same for the test
>> files).
>> >
>> >> > +
>> >> > +    # Read PL3_SSP register.
>> >> > +    set ssp_main [get_hexadecimal_valueof "\$pl3_ssp" "read
>> >> > + pl3_ssp value"]
>> >> > +
>> >> > +    # Write PL3_SSP register.
>> >> > +    gdb_test "print /x \$pl3_ssp = 0x12345678" "= 0x12345678" "set
>> >> > + pl3_ssp
>> >> value"
>> >> > +    gdb_test "print /x \$pl3_ssp" "= 0x12345678" "read pl3_ssp
>> >> > + value after
>> >> setting"
>> >> > +
>> >> > +    # Restore original value.
>> >> > +    gdb_test "print /x \$pl3_ssp = $ssp_main" "= $ssp_main"
>> >> > + "restore
>> >> original pl3_ssp"
>> >> > +
>> >> > +    # Potential CET violations often only occur after resuming
>> >> > + normal
>> >> execution.
>> >> > +    # Therefore, it is important to test normal program continuation
>> after
>> >> > +    # configuring the shadow stack pointer.
>> >> > +    gdb_continue_to_end
>> >>
>> >> I assume that if we continue with the bogus value in place the
>> >> inferior would either give an error or terminate.  Is it worth trying
>> >> this and checking that the inferior behaves as expected?
>> >
>> > If we don't reset the shadow stack pointer to it's original value we will see
>> a SEGV.
>> > Dependent on the address of the wrong shadow stack pointer it's either
>> > a SEGV with si code that points to a control flow protection fault or a
>> different si code.
>> >
>> > So if I stay in a valid address range for configuring pl3_ssp but
>> > don't restore the original value I'll see a control flow protection exception:
>> >
>> > [...]
>> > breakpoint 1, 0x0000555555555148 in main ()^M
>> > (gdb) print /x $pl3_ssp^M
>> > $1 = 0x7ffff7bfffe8^M
>> > (gdb) PASS: gdb.arch/amd64-ssp.exp: get hexadecimal valueof "$pl3_ssp"
>> > print /x $pl3_ssp = 0x7ffff7bfffe0^M
>> > $2 = 0x7ffff7bfffe0^M
>> > (gdb) PASS: gdb.arch/amd64-ssp.exp: set pl3_ssp value print /x
>> > $pl3_ssp^M
>> > $3 = 0x7ffff7bfffe0^M
>> > (gdb) PASS: gdb.arch/amd64-ssp.exp: read pl3_ssp value after setting
>> > continue^M Continuing.^M ^M Program received signal SIGSEGV,
>> > Segmentation fault.^M
>> > 0x0000555555555158 in main ()^M
>> > (gdb) FAIL: gdb.arch/amd64-ssp.exp: continue until exit
>> >
>> > Siginfo shows si_code = 10, which indicates a control protection fault.
>> >
>> > p $_siginfo^M
>> > $4 = {si_signo = 11, si_errno = 0, si_code = 10, [...]
>> >
>> > If I set the value of pl3_ssp as in the current test (0x12345678) I'll
>> > see a different SEGV actually
>> >
>> > p $_siginfo
>> > $4 = {si_signo = 11, si_errno = 0, si_code = 1, [...]
>> >
>> >>
>> >> What if, say, the $pl3_ssp value only ever made it as far as the
>> >> register cache, and was never actually written back to the inferior?
>> >> I don't think the above test would actually spot this bug, right?
>> >
>> > Hm, if I understand you correctly here and you mean the scenario as
>> > shown above the above test would spot this bug I think (as we saw a fail).
>> >
>> > Does my example above show what you described or do you mean a
>> > different scenario?
>> 
>> Yes, something like the above would check that the register is actually being
>> written back to the hardware, and is written to the expected location.
>> 
>> The current test, as written in the patch, writes a bad value to the shadow
>> stack, then restores the correct value.  What if the bad value never actually
>> got written back to the hardware at all, and was just being held in the
>> register cache?
>> 
>> Having a test that writes a bad value, then does 'continue', and expects to
>> see something like 'Program received signal ...' would be a reasonable
>> indication that the write to the shadow stack actually made it to the h/w.
>> 
>> Thanks,
>> Andrew
>
>
> Yes, I agree, I'll add:
>
> ~~~
>     with_test_prefix "invalid ssp" {
> 	write_invalid_ssp
>
> 	# Continue until SIGSEV to test that the value is written back to HW.
> 	gdb_test "continue" \
> 	    [multi_line \
> 		"Continuing\\." \
> 		"" \
> 		"Program received signal SIGSEGV, Segmentation fault\\." \
> 		"$hex in main \\(\\)"] \
> 	    "continue to SIGSEGV"
>     }
>
>     clean_restart ${binfile}
>     if { ![runto_main] } {
> 	return -1
>     }
>
>     with_test_prefix "restore original ssp" {
> 	# Read PL3_SSP register.
> 	set ssp_main [get_hexadecimal_valueof "\$pl3_ssp" "read pl3_ssp value"]
>
> 	write_invalid_ssp
>
> 	# Restore original value.
> 	gdb_test "print /x \$pl3_ssp = $ssp_main" "= $ssp_main" "restore original value"
>
> 	# Now we should not see a SIGSEV, since the original value is restored.
> 	gdb_continue_to_end
>     }

Sorry for the late reply.  This test looks great, exactly what I
imagined.

Thanks,
Andrew