Re: [PATCH v5 07/12] gdb: amd64 linux coredump support with shadow stack.

[Andrew Burgess <aburgess@redhat.com>]

Christina Schimpe <christina.schimpe@intel.com> writes:

> Intel's Control-Flow Enforcement Technology (CET) provides the shadow
> stack feature for the x86 architecture.
>
> This commit adds support to write and read the shadow-stack node in
> corefiles.  This helps debugging return address violations post-mortem.
> The format is synced with the linux kernel commit "x86: Add PTRACE
> interface for shadow stack".  As the linux kernel restricts shadow
> stack support to 64-bit, apply the fix for amd64 only.
>
> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  gdb/amd64-linux-tdep.c                        |  57 ++++++++-
>  .../gdb.arch/amd64-shadow-stack-corefile.c    |  42 +++++++
>  .../gdb.arch/amd64-shadow-stack-corefile.exp  | 110 ++++++++++++++++++
>  3 files changed, 205 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.exp
>
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index edb7d8da6ab..9af7a41ea26 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -47,6 +47,7 @@
>  #include "expop.h"
>  #include "arch/amd64-linux-tdesc.h"
>  #include "inferior.h"
> +#include "x86-tdep.h"
>  
>  /* The syscall's XML filename for i386.  */
>  #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
> @@ -1593,6 +1594,14 @@ amd64_linux_record_signal (struct gdbarch *gdbarch,
>    return 0;
>  }
>  
> +/* Get shadow stack pointer state from core dump.  */
> +
> +static bool
> +amd64_linux_core_read_ssp_state_p (bfd *abfd)
> +{
> +  return bfd_get_section_by_name (abfd, ".reg-ssp") != nullptr;

I think the comment on this function is not correct.  We're not getting
the shadow stack pointer state, we're:

  /* Return true if the core file ABFD contains shadow stack pointer
     state.  Otherwise, return false.  */


> +}
> +
>  /* Get Linux/x86 target description from core dump.  */
>  
>  static const struct target_desc *
> @@ -1602,11 +1611,14 @@ amd64_linux_core_read_description (struct gdbarch *gdbarch,
>  {
>    /* Linux/x86-64.  */
>    x86_xsave_layout layout;
> -  uint64_t xcr0 = i386_linux_core_read_xsave_info (abfd, layout);
> -  if (xcr0 == 0)
> -    xcr0 = X86_XSTATE_SSE_MASK;
> +  uint64_t xstate_bv_mask = i386_linux_core_read_xsave_info (abfd, layout);

As with feedback on earlier patches, I don't think the inclusion of
'_mask' is a good one here.  I think what you call xstate_bv_mask is an
actual value, not a mask, and should be named 'xstate_bv'

> +  if (xstate_bv_mask == 0)
> +    xstate_bv_mask = X86_XSTATE_SSE_MASK;
> +
> +  if (amd64_linux_core_read_ssp_state_p (abfd))
> +    xstate_bv_mask |= X86_XSTATE_CET_U;
>  
> -  return amd64_linux_read_description (xcr0 & X86_XSTATE_ALL_MASK,
> +  return amd64_linux_read_description (xstate_bv_mask & X86_XSTATE_ALL_MASK,
>  				       gdbarch_ptr_bit (gdbarch) == 32);
>  }
>  
> @@ -1637,6 +1649,35 @@ static const struct regset amd64_linux_xstateregset =
>      amd64_linux_collect_xstateregset
>    };
>  
> +/* Supply shadow stack pointer register from SSP to the register cache
> +   REGCACHE.  */
> +
> +static void
> +amd64_linux_supply_ssp (const regset *regset,
> +			regcache *regcache, int regnum,
> +			const void *ssp, size_t len)
> +{
> +  x86_supply_ssp (regcache, *static_cast<const uint64_t *> (ssp));

Before this line I'd like to see:

  gdb_assert (len == sizeof (uint64_t));

> +}
> +
> +/* Collect the shadow stack pointer register from the register cache
> +   REGCACHE and store it in SSP.  */
> +
> +static void
> +amd64_linux_collect_ssp (const regset *regset,
> +			 const regcache *regcache, int regnum,
> +			 void *ssp, size_t len)
> +{
> +  x86_collect_ssp (regcache, *static_cast<uint64_t *> (ssp));

And I think this should get the same 'len == sizeof ...' assert as
above please.

> +}
> +
> +/* Shadow stack pointer register.  */
> +
> +static const struct regset amd64_linux_ssp_register
> +  {
> +    NULL, amd64_linux_supply_ssp, amd64_linux_collect_ssp
> +  };
> +
>  /* Iterate over core file register note sections.  */
>  
>  static void
> @@ -1653,6 +1694,14 @@ amd64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>      cb (".reg-xstate", tdep->xsave_layout.sizeof_xsave,
>  	tdep->xsave_layout.sizeof_xsave, &amd64_linux_xstateregset,
>  	"XSAVE extended state", cb_data);
> +
> +  /* SSP can be unavailable.  Thus, we need to check the register status
> +     in case we write a core file (regcache != nullptr).  */
> +  if (tdep->ssp_regnum > 0
> +      && (regcache == nullptr

Have you really seen this function called with 'regcache == nullptr' ?
I'm suspect not as e.g. i386_supply_gregset, which is part of
i386_gregset, makes an unchecked dereference of regcache, so if regcache
was nullptr you'd see UB (crash) before getting to this check.

> +	  || REG_VALID == regcache->get_register_status (tdep->ssp_regnum)))
> +    cb (".reg-ssp", 8, 8, &amd64_linux_ssp_register,
> +	"shadow stack pointer", cb_data);
>  }
>  
>  /* The instruction sequences used in x86_64 machines for a
> diff --git a/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c b/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c
> new file mode 100644
> index 00000000000..f078e33810d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c
> @@ -0,0 +1,42 @@
> +/* This test program is part of GDB, the GNU debugger.
> +
> +   Copyright 2025 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/>.  */
> +
> +#include <stdio.h>
> +
> +/* Call the return instruction before function epilogue to trigger a
> +   control-flow exception.  */
> +void
> +function ()
> +{
> +  unsigned long ssp;
> +  asm volatile("xor %0, %0; rdsspq %0" : "=r" (ssp));
> +
> +  /* Print ssp to stdout so that the testcase can capture it.  */
> +  printf ("%p\n", (void *) ssp);
> +  fflush (stdout);
> +
> +  /* Manually cause a control-flow exception by executing a return
> +     instruction before function epilogue, so the address atop the stack
> +     is not the return instruction.  */
> +  __asm__ volatile ("ret\n");
> +}
> +
> +int
> +main (void)
> +{
> +  function (); /* Break here.  */
> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.exp b/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.exp
> new file mode 100644
> index 00000000000..8784fc3622c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.exp
> @@ -0,0 +1,110 @@
> +# Copyright 2021-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 the shadow stack pointer note in core dumps.
> +
> +require allow_ssp_tests
> +
> +standard_testfile
> +
> +proc check_core_file {core_filename saved_pl3_ssp} {
> +    global decimal
> +
> +    # Load the core file.
> +    if [gdb_test "core $core_filename" \
> +	    [multi_line \
> +		 "Core was generated by .*\\." \
> +		 "Program terminated with signal SIGSEGV, Segmentation fault.*" \
> +		 "#0  function \\(\\) at .*amd64-shadow-stack-corefile.c:$decimal" \
> +		 "$decimal.*__asm__ volatile \\(\"ret\\\\n\"\\);"] \
> +	    "load core file"] {
> +	return
> +    }
> +
> +    # Check the value of ssp in the core file.
> +    gdb_test "print/x \$pl3_ssp" "\\$\[0-9\]+ = $saved_pl3_ssp" \
> +	"pl3_ssp contents from core file $saved_pl3_ssp"
> +}
> +
> +save_vars { ::env(GLIBC_TUNABLES) } {
> +
> +    append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
> +
> +    if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  {debug additional_flags="-fcf-protection=return"}] } {
> +	return
> +    }
> +
> +    set linespec ${srcfile}:[gdb_get_line_number "Break here"]
> +
> +    if ![runto $linespec] {
> +	return
> +    }
> +
> +    # Continue until a crash.  The line with the hex number is optional because
> +    # it's printed by the test program, and doesn't appear in the Expect buffer
> +    # when testing a remote target.
> +    gdb_test "continue" \
> +	[multi_line \
> +	     "Continuing\\." \
> +	     "($hex\r\n)?" \
> +	     "Program received signal SIGSEGV, Segmentation fault.*" \
> +	     "function \\(\\) at .*amd64-shadow-stack-corefile.c:$decimal" \
> +	     {.*__asm__ volatile \("ret\\n"\);}] \
> +    "continue to SIGSEGV"
> +
> +    set ssp_in_gcore [get_valueof "/x" "\$pl3_ssp" "*unknown*"]
> +
> +    # Generate the gcore core file.
> +    set gcore_filename [standard_output_file "${testfile}.gcore"]
> +    set gcore_generated [gdb_gcore_cmd "$gcore_filename" "generate gcore file"]
> +
> +    # Obtain an OS-generated core file.  Save test program output to
> +    # ${binfile}.out.
> +    set core_filename [core_find $binfile {} {} "${binfile}.out"]
> +    set core_generated [expr {$core_filename != ""}]
> +    set os_core_name "${binfile}.core"
> +    remote_exec build "mv $core_filename $os_core_name"
> +    set core_filename $os_core_name

I'm wondering what the point of this core_filename / os_core_name stuff
is?  My reading of `core_find` is that the returned core_filename will
be '${binfile}.core', so this whole thing feels redundant, but maybe I'm
missing something here?

> +
> +    # At this point we have a couple of core files, the gcore one generated by
> +    # GDB and the one generated by the operating system.  Make sure GDB can
> +    # read both correctly.
> +
> +    if {$gcore_generated} {
> +	clean_restart $binfile
> +
> +	with_test_prefix "gcore corefile" {
> +	    check_core_file $gcore_filename $ssp_in_gcore
> +	}
> +    } else {
> +	fail "gcore corefile not generated"

It's better, where possible, to avoid having pass/fail results that only
show up down some code paths.

In this case it's easy to avoid having a stray 'fail' by restructuring
the code too:

  gdb_assert { $gcore_generated } "gcore corefile created"
  if { $gcore_generated } {
    ... etc ...
  }

Now you'll always have either a pass or fail based on the gcore being
generated.

There is also the helper proc `gcore_cmd_available`.  I'd guess for any
x86 target that supports SSP, gcore will be available, but in theory you
could consider using this to avoid a fail when gcore is not available
maybe?

> +    }
> +
> +    if {$core_generated} {
> +	clean_restart $binfile
> +
> +	with_test_prefix "OS corefile" {
> +	    # Read ssp value from saved output of the test program.
> +	    set out_id [open ${binfile}.out "r"]
> +	    set ssp_in_gcore [gets $out_id]
> +
> +	    close $out_id
> +	    check_core_file $core_filename $ssp_in_gcore

I'd move the blank line after the 'close' personally.

Thanks,
Andrew


> +	}
> +    } else {
> +	untested "OS corefile not generated"
> +    }
> +}
> -- 
> 2.43.0