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,
>
> 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.

>
>> > +
>> >    /* 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




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