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

[Andrew Burgess <aburgess@redhat.com>]

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.

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

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.


> +
>  @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?

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


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

> +
> +    # 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?

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?

> +}
> +

Thanks,
Andrew