Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Luis Machado <luis.machado@arm.com>
To: Christina Schimpe <christina.schimpe@intel.com>,
	gdb-patches@sourceware.org
Cc: thiago.bauermann@linaro.org, eliz@gnu.org
Subject: Re: [PATCH v4 05/11] gdb, gdbserver: Add support of Intel shadow stack pointer register.
Date: Thu, 19 Jun 2025 10:24:22 +0100	[thread overview]
Message-ID: <fabd5ff5-c2a3-427c-99d4-7a1296f2e5d7@arm.com> (raw)
In-Reply-To: <20250617121147.1956686-6-christina.schimpe@intel.com>

On 6/17/25 13:11, Christina Schimpe wrote:
> 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>
> ---
>  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                           | 55 +++++++++++++++
>  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                           | 50 +++++++++++--
>  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, 446 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/nat/x86-linux.c b/gdb/nat/x86-linux.c
> index 0bdff736f8a..bf603182164 100644
> --- a/gdb/nat/x86-linux.c
> +++ b/gdb/nat/x86-linux.c
> @@ -17,6 +17,12 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> +#include "elf/common.h"
> +#include "gdbsupport/common-defs.h"
> +#include "nat/gdb_ptrace.h"
> +#include "nat/linux-ptrace.h"
> +#include "nat/x86-cpuid.h"
> +#include <sys/uio.h>
>  #include "x86-linux.h"
>  #include "x86-linux-dregs.h"
>  #include "nat/gdb_ptrace.h"
> @@ -126,3 +132,52 @@ x86_linux_ptrace_get_arch_size (int tid)
>    return x86_linux_arch_size (false, false);
>  #endif
>  }
> +
> +bool
> +x86_check_ssp_support (const int tid)

Missing the usual comment pointing at the header file?

/* See nat/x86-linux.h.  */

> +{
> +  /* It's not enough to check shadow stack support with the ptrace call
> +     below only, as we cannot distinguish between shadow stack not enabled
> +     for the current thread and shadow stack is not supported by HW.  In
> +     both scenarios the ptrace call fails with ENODEV.  In case shadow
> +     stack is not enabled for the current thread, we still want to return
> +     true.  */
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  __get_cpuid_count (7, 0, &eax, &ebx, &ecx, &edx);
> +
> +  if ((ecx & bit_SHSTK) == 0)
> +    return false;
> +
> +  /* Further check for NT_X86_SHSTK kernel support.  */
> +  uint64_t ssp;
> +  iovec iov {&ssp, sizeof (ssp) };
> +
> +  errno = 0;
> +  int res = ptrace (PTRACE_GETREGSET, tid, NT_X86_SHSTK, &iov);
> +  if (res < 0)
> +    {
> +      if (errno == EINVAL)
> +	{
> +	  /* The errno EINVAL for a PTRACE_GETREGSET call indicates that
> +	     kernel support is not available.  */
> +	  return false;
> +	}
> +      else if (errno == ENODEV)
> +	{
> +	  /* At this point, since we already checked CPUID, the errno
> +	     ENODEV for a PTRACE_GETREGSET call indicates that shadow
> +	     stack is not enabled for the current thread.  As it could be
> +	     enabled later, we still want to return true here.  */
> +	  return true;
> +	}
> +      else
> +	{
> +	  warning (_("Unknown ptrace error for NT_X86_SHSTK: %s"),
> +		   safe_strerror (errno));
> +	  return false;
> +	}
> +    }
> +
> +  return true;
> +}

<...>

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 3f1cd55d727..7e56cf61a4c 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4287,6 +4287,76 @@ gdb_caching_proc allow_tsx_tests {} {
>      return $allow_tsx_tests
>  }
>  
> +# Run a test on the target to check if it supports x86 shadow stack.  Return 1
> +# if shadow stack is enabled, 0 otherwise.
> +
> +gdb_caching_proc allow_ssp_tests {} {
> +    global srcdir subdir gdb_prompt hex
> +
> +    set me "allow_ssp_tests"
> +
> +    if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +	verbose "$me: target known to not support shadow stack."
> +	return 0
> +    }
> +
> +    # There is no need to check the actual HW in addition to ptrace support.
> +    # We need both checks and ptrace will tell us about the HW state.
> +    set compile_flags "{additional_flags=-fcf-protection=return}"
> +    set src { int main() { return 0; } }
> +    if {![gdb_simple_compile $me $src executable $compile_flags]} {
> +	return 0
> +    }
> +
> +    save_vars { ::env(GLIBC_TUNABLES) } {
> +
> +	append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
> +
> +	# No error message, compilation succeeded so now run it via gdb.
> +	gdb_exit
> +	gdb_start
> +	gdb_reinitialize_dir $srcdir/$subdir
> +	gdb_load $obj
> +	if {![runto_main]} {
> +	    remote_file build delete $obj
> +	    return 0
> +	}
> +	set shadow_stack_disabled_re "(<unavailable>)"
> +	if {[istarget *-*-linux*]} {
> +	    # Starting with v6.6., the Linux kernel supports CET shadow stack.

Small typo, period after v6.6.

> +	    # Dependent on the target we can see a nullptr or "<unavailable>"
> +	    # when shadow stack is supported by HW and the Linux kernel but
> +	    # not enabled for the current thread (for example due to a lack
> +	    # of compiler or glibc support for -fcf-protection).
> +	    set shadow_stack_disabled_re "$shadow_stack_disabled_re|(.*0x0)"
> +	}
> +
> +	set allow_ssp_tests 0
> +	gdb_test_multiple "print \$pl3_ssp" "test shadow stack support" {
> +	    -re -wrap "(.*$hex)((?!(.*0x0)).)" {
> +		verbose -log "$me: Shadow stack support detected."
> +		set allow_ssp_tests 1
> +	    }
> +	    -re -wrap $shadow_stack_disabled_re {
> +		# In case shadow stack is not enabled (for example due to a
> +		# lack of compiler or glibc support for -fcf-protection).
> +		verbose -log "$me: Shadow stack is not enabled."
> +	    }
> +	    -re -wrap "void" {
> +		# In case we don't have hardware or kernel support.
> +		verbose -log "$me: No shadow stack support."
> +	    }
> +	}
> +
> +	gdb_exit
> +    }
> +
> +    remote_file build delete $obj
> +
> +    verbose "$me: returning $allow_ssp_tests" 2
> +    return $allow_ssp_tests
> +}
> +
>  # Run a test on the target to see if it supports avx512bf16.  Return 1 if so,
>  # 0 if it does not.  Based on 'check_vmx_hw_available' from the GCC testsuite.
>  
> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
> index a82ad21da27..865c017404e 100644
> --- a/gdb/x86-linux-nat.c
> +++ b/gdb/x86-linux-nat.c
> @@ -41,6 +41,7 @@
>  #include "nat/x86-linux.h"
>  #include "nat/x86-linux-dregs.h"
>  #include "nat/linux-ptrace.h"
> +#include "x86-tdep.h"
>  #include "nat/x86-linux-tdesc.h"
>  
>  /* linux_nat_target::low_new_fork implementation.  */
> @@ -97,11 +98,10 @@ const struct target_desc *
>  x86_linux_nat_target::read_description ()
>  {
>    /* The x86_linux_tdesc_for_tid call only reads xcr0 the first time it is
> -     called.  The mask is stored in XSTATE_BV_STORAGE and reused on
> -     subsequent calls.  Note that GDB currently supports features for user
> -     state components only.  However, once supervisor state components are
> -     supported in GDB XSTATE_BV_STORAGE will not be configured based on
> -     xcr0 only.  */
> +     called.  Also it checks the enablement state of features which are
> +     not configured in xcr0, such as CET shadow stack.  Once the supported
> +     features are identified, the XSTATE_BV_STORAGE value is configured
> +     accordingly and preserved for subsequent calls of this function.  */
>    static uint64_t xstate_bv_storage;
>  
>    if (inferior_ptid == null_ptid)
> @@ -215,6 +215,46 @@ x86_linux_get_thread_area (pid_t pid, void *addr, unsigned int *base_addr)
>  }
>  \f
>  
> +/* See x86-linux-nat.h.  */
> +
> +void
> +x86_linux_fetch_ssp (regcache *regcache, const int tid)
> +{
> +  uint64_t ssp = 0x0;
> +  iovec iov {&ssp, sizeof (ssp)};
> +
> +  /* The shadow stack may be enabled and disabled at runtime.  Reading the
> +     ssp might fail as shadow stack was not activated for the current
> +     thread.  We don't want to show a warning but silently return.  The
> +     register will be shown as unavailable for the user.  */
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_X86_SHSTK, &iov) != 0)
> +    return;
> +
> +  x86_supply_ssp (regcache, ssp);
> +}
> +
> +/* See x86-linux-nat.h.  */
> +
> +void
> +x86_linux_store_ssp (const regcache *regcache, const int tid)
> +{
> +  uint64_t ssp = 0x0;
> +  iovec iov {&ssp, sizeof (ssp)};
> +  x86_collect_ssp (regcache, ssp);
> +
> +  /* Starting with v6.6., the Linux kernel supports CET shadow stack.

Same typo, period after v6.6

> +     Dependent on the target the ssp register can be unavailable or
> +     nullptr when shadow stack is supported by HW and the Linux kernel but
> +     not enabled for the current thread.  In case of nullptr, GDB tries to
> +     restore the shadow stack pointer after an inferior call.  The ptrace
> +     call with PTRACE_SETREGSET will fail here with errno ENODEV.  We
> +     don't want to throw an error in this case but silently continue.  */
> +  errno = 0;
> +  if ((ptrace (PTRACE_SETREGSET, tid, NT_X86_SHSTK, &iov) != 0)
> +      && (errno != ENODEV))
> +    perror_with_name (_("Failed to write pl3_ssp register"));
> +}
> +
>  void _initialize_x86_linux_nat ();
>  void
>  _initialize_x86_linux_nat ()

<...>

Other than the cosmetic points above, the patch looks OK to me. I'm not entirely familiar
with the feature for x86, but the enablement makes sense to me.

Reviewed-By: Luis Machado <luis.machado@arm.com>

  parent reply	other threads:[~2025-06-19  9:28 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-17 12:11 [PATCH v4 00/11] Add CET shadow stack support Christina Schimpe
2025-06-17 12:11 ` [PATCH v4 01/11] gdbserver: Add optional runtime register set type Christina Schimpe
2025-06-19  9:27   ` Luis Machado
2025-06-17 12:11 ` [PATCH v4 02/11] gdbserver: Add assert in x86_linux_read_description Christina Schimpe
2025-06-19  9:27   ` Luis Machado
2025-06-17 12:11 ` [PATCH v4 03/11] gdb: Sync up x86-gcc-cpuid.h with cpuid.h from gcc 14 branch Christina Schimpe
2025-06-17 18:12   ` Tom Tromey
2025-06-20 12:39     ` Schimpe, Christina
2025-06-17 12:11 ` [PATCH v4 04/11] gdb, gdbserver: Use xstate_bv for target description creation on x86 Christina Schimpe
2025-06-19  9:23   ` Luis Machado
2025-06-23 12:46     ` Schimpe, Christina
2025-06-23 12:56       ` Luis Machado
2025-06-24 13:46         ` Schimpe, Christina
2025-06-26 16:03           ` Luis Machado
2025-06-17 12:11 ` [PATCH v4 05/11] gdb, gdbserver: Add support of Intel shadow stack pointer register Christina Schimpe
2025-06-17 12:20   ` Eli Zaretskii
2025-06-19  9:24   ` Luis Machado [this message]
2025-06-23 13:05     ` Schimpe, Christina
2025-06-17 12:11 ` [PATCH v4 06/11] gdb: amd64 linux coredump support with shadow stack Christina Schimpe
2025-06-19  9:24   ` Luis Machado
2025-06-23 13:16     ` Schimpe, Christina
2025-06-17 12:11 ` [PATCH v4 07/11] gdb: Handle shadow stack pointer register unwinding for amd64 linux Christina Schimpe
2025-06-19  9:25   ` Luis Machado
2025-06-20  1:42     ` Thiago Jung Bauermann
2025-06-23 14:55       ` Schimpe, Christina
2025-06-23 23:26         ` Thiago Jung Bauermann
2025-06-23 15:00     ` Schimpe, Christina
2025-06-23 15:06       ` Luis Machado
2025-06-23 23:36         ` Thiago Jung Bauermann
2025-06-20  1:52   ` Thiago Jung Bauermann
2025-06-17 12:11 ` [PATCH v4 08/11] gdb, gdbarch: Enable inferior calls for shadow stack support Christina Schimpe
2025-06-19  9:25   ` Luis Machado
2025-06-23 17:49     ` Schimpe, Christina
2025-06-17 12:11 ` [PATCH v4 09/11] gdb: Implement amd64 linux shadow stack support for inferior calls Christina Schimpe
2025-06-17 12:21   ` Eli Zaretskii
2025-06-19  9:25   ` Luis Machado
2025-06-27 19:52     ` Schimpe, Christina
2025-06-28 10:38       ` Luis Machado
2025-06-28 20:03         ` Thiago Jung Bauermann
2025-06-28 21:05           ` Thiago Jung Bauermann
2025-06-17 12:11 ` [PATCH v4 10/11] gdb, gdbarch: Introduce gdbarch method to get the shadow stack pointer Christina Schimpe
2025-06-17 18:16   ` Tom Tromey
2025-06-20 12:59     ` Schimpe, Christina
2025-06-19  9:26   ` Luis Machado
2025-06-23 18:00     ` Schimpe, Christina
2025-06-17 12:11 ` [PATCH v4 11/11] gdb: Enable displaced stepping with shadow stack on amd64 linux Christina Schimpe
2025-06-17 12:22   ` Eli Zaretskii
2025-06-17 15:16     ` Schimpe, Christina
2025-06-19  9:26   ` Luis Machado
2025-06-23 18:24     ` Schimpe, Christina
2025-06-24  8:05       ` Luis Machado
2025-06-27 19:26         ` Schimpe, Christina
2025-06-28 10:35           ` Luis Machado

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fabd5ff5-c2a3-427c-99d4-7a1296f2e5d7@arm.com \
    --to=luis.machado@arm.com \
    --cc=christina.schimpe@intel.com \
    --cc=eliz@gnu.org \
    --cc=gdb-patches@sourceware.org \
    --cc=thiago.bauermann@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox