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

~~~

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