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

Andrew Burgess
2025-08-14T11:40:36Z

"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. -->
>> >> > +
>> >> > + <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. -->
>> >> > +
>> >> > + <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