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

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Andrew, 

Thank you for the feedback. Please find my comments to your feedback below.
I'll only comment on the non-test part, since for the test I already replied in:
https://sourceware.org/pipermail/gdb-patches/2025-July/219525.html


> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Tuesday, July 29, 2025 4:47 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 07/12] gdb: amd64 linux coredump support with
> shadow stack.
> 
> 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.  */

Agree, will add.

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

I agree and will fix.

> > +  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));

Agree, will add.

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

Agree, will add.

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

I'll change the 

"tdep->ssp_regnum > 0"

to

"tdep->ssp_regnum != -1"

as discussed already.

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


We see this if we load a corefile:
 ~~~
(gdb) core core.3499583
[New LWP 3499583]
Reading symbols from /tmp/main...

Thread 1 "gdb-up" hit Breakpoint 1, amd64_linux_iterate_over_regset_sections (gdbarch=0x5555589cc140, 
    cb=0x555555fdd64f <get_core_registers_cb(char const*, int, int, regset const*, char const*, void*)>, cb_data=0x7fffffffad20, regcache=0x0)
    at /tmp/gdb/amd64-linux-tdep.c:1694
1694	  i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch)
(gdb) bt
#0  amd64_linux_iterate_over_regset_sections (gdbarch=0x5555589cc140, cb=0x555555fdd64f <get_core_registers_cb(char const*, int, int, regset const*, char const*, void*)>, 
    cb_data=0x7fffffffad20, regcache=0x0) at / tmp/gdb/amd64-linux-tdep.c:1694
#1  0x0000555555e15491 in gdbarch_iterate_over_regset_sections (gdbarch=0x5555589cc140, cb=0x555555fdd64f <get_core_registers_cb(char const*, int, int, regset const*, char const*, void*)>, 
    cb_data=0x7fffffffad20, regcache=0x0) at /tmp/gdb/gdbarch-gen.c:3840
#2  0x0000555555fdd876 in core_target::fetch_registers (this=0x5555588ee4e0, regcache=0x5555589c9120, regno=16) at /tmp /gdb/corelow.c:1425
#3  0x0000555556744d0a in target_fetch_registers (regcache=0x5555589c9120, regno=16) at /tmp/gdb/target.c:3915
[...]
~~~

In corelow.c: core_target::fetch_registers we can also see that that we pass the nullptr:
~~~
  struct gdbarch *gdbarch = regcache->arch ();
  get_core_registers_cb_data data = { this, regcache };
  gdbarch_iterate_over_regset_sections (gdbarch,
					get_core_registers_cb,
					(void *) &data, NULL);
~~~

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

Kind 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