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

Andrew Burgess
2025-07-29T14:47:33Z

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