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

Schimpe, Christina
2025-08-04T12:46:21Z

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