RE: [PATCH v5 12/12] gdb: Enable displaced stepping with shadow stack on amd64 linux.

Schimpe, Christina
2025-07-31T17:31:16Z

Hi Andrew,

Thank you for the review.

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, July 30, 2025 4:00 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 12/12] gdb: Enable displaced stepping with shadow
> stack on amd64 linux.
>
> Christina Schimpe <christina.schimpe@intel.com> writes:
>
> > Currently, if displaced stepping is active and the single stepped
> > instruction is a call instruction, the return address atop the stack
> > is the address following the copied instruction. However, to allow
> > normal program execution it has to be the address following the
> > original instruction. Due to that reason, the return address is
> > corrected in amd64_displaced_step_fixup and i386_displaced_step_fixup.
> >
> > For programs that are shadow-stack enabled we see a control-protection
> > exception, as the address on the shadow stack does not match the
> > address atop the stack.
> >
> > Fix this by correcting the shadow stack top address as well.
> >
> > Reviewed-By: Luis Machado <luis.machado@arm.com>
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> > ---
> > gdb/NEWS | 3 +
> > gdb/amd64-linux-tdep.c | 16 +++-
> > gdb/amd64-tdep.c | 15 +++
> > gdb/doc/gdb.texinfo | 11 ++-
> > gdb/i386-tdep.c | 15 +++
> > .../gdb.arch/amd64-shadow-stack-disp-step.exp | 92
> > +++++++++++++++++++
> > 6 files changed, 149 insertions(+), 3 deletions(-) create mode
> > 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-disp-step.exp
> >
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index ba555f0dea1..60510fefea4 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -3,6 +3,9 @@
> >
> > *** Changes since GDB 16
> >
> > +* Debugging Linux programs that use x86-64 or x86-64 with 32-bit
> > +pointer
> > + size (X32) Shadow Stacks are now supported.
> > +
> > * Support for the shadow stack pointer register on x86-64 or x86-64 with
> > 32-bit pointer size (X32) GNU/Linux.
> >
> > diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index
> > 899fe2df02c..782b66f1467 100644
> > --- a/gdb/amd64-linux-tdep.c
> > +++ b/gdb/amd64-linux-tdep.c
> > @@ -1936,8 +1936,10 @@
> amd64_linux_shadow_stack_element_size_aligned (gdbarch *gdbarch)
> > possible. */
> >
> > static std::optional<CORE_ADDR>
> > -amd64_linux_get_shadow_stack_pointer (gdbarch *gdbarch, regcache
> > *regcache)
> > +amd64_linux_get_shadow_stack_pointer (gdbarch *gdbarch, regcache
> *regcache,
> > + bool &shadow_stack_enabled)
> > {
> > + shadow_stack_enabled = false;
> > const i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep>
> > (gdbarch);
> >
> > if (tdep == nullptr || tdep->ssp_regnum < 0) @@ -1955,6 +1957,9 @@
> > amd64_linux_get_shadow_stack_pointer (gdbarch *gdbarch, regcache
> *regcache)
> > if (ssp == 0x0)
> > return {};
> >
> > + /* In case there is a shadow stack pointer available which is non-null,
> > + the shadow stack feature is enabled. */ shadow_stack_enabled =
> > + true;
> > return ssp;
> > }
> >
> > @@ -1965,8 +1970,13 @@ static void
> > amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR
> new_addr,
> > regcache *regcache)
> > {
> > + bool shadow_stack_enabled;
> > std::optional<CORE_ADDR> ssp
> > - = amd64_linux_get_shadow_stack_pointer (gdbarch, regcache);
> > + = amd64_linux_get_shadow_stack_pointer (gdbarch, regcache,
> > + shadow_stack_enabled);
> > +
> > + /* For amd64/Linux, if SSP has a value that means shadow stack is
> > + enabled. */
>
> It feels like this statement should be associated with an assert. I'd
> suggest:
>
> if (!ssp.has_value ())
> return;
> else
> gdb_assert (shadow_stack_enabled);

Agree, will add.

> > if (!ssp.has_value ())
> > return;
> >
> > @@ -2122,6 +2132,8 @@ amd64_linux_init_abi_common(struct
> gdbarch_info info, struct gdbarch *gdbarch,
> > (gdbarch, amd64_linux_remove_non_address_bits_watchpoint);
> >
> > set_gdbarch_shadow_stack_push (gdbarch,
> > amd64_linux_shadow_stack_push);
> > + set_gdbarch_get_shadow_stack_pointer (gdbarch,
> > +
> amd64_linux_get_shadow_stack_pointer);
> > dwarf2_frame_set_init_reg (gdbarch, amd64_init_reg); }
> >
> > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index
> > 450dbc38047..8afb3a7abba 100644
> > --- a/gdb/amd64-tdep.c
> > +++ b/gdb/amd64-tdep.c
> > @@ -1917,6 +1917,21 @@ amd64_displaced_step_fixup (struct gdbarch
> *gdbarch,
> > displaced_debug_printf ("relocated return addr at %s to %s",
> > paddress (gdbarch, rsp),
> > paddress (gdbarch, retaddr));
> > +
> > + /* If shadow stack is enabled, we need to correct the return address
> > + on the shadow stack too. */
> > + bool shadow_stack_enabled;
> > + std::optional<CORE_ADDR> ssp
> > + = gdbarch_get_shadow_stack_pointer (gdbarch, regs,
> > + shadow_stack_enabled);
> > + if (ssp.has_value () && shadow_stack_enabled)
>
> Given the strengthening of the comment on
> gdbarch_get_shadow_stack_pointer that I suggest in the previous patch, I
> think this should become:
>
> if (shadow_stack_enabled)
> {
> gdb_assert (ssp.has_value ());
>
> ... etc ...
> }

Agree, will add.

> > + {
> > + write_memory_unsigned_integer (*ssp, retaddr_len, byte_order,
> > + retaddr);
> > + displaced_debug_printf ("relocated shadow stack return addr at %s
> "
> > + "to %s", paddress (gdbarch, *ssp),
> > + paddress (gdbarch, retaddr));
> > + }
> > }
> > }
> >
> > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index
> > b5120b78426..488816d5ca2 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -27059,12 +27059,20 @@ the program stream must be an
> @code{ENDBR}
> > instruction, otherwise the processor signals a control protection
> exception.
> > @end itemize
> >
> > -Impact on Call/Print:
> > +Impact on GDB commands:
> > +@itemize @bullet
> > +@item Call/Print:
> > Inferior calls in @value{GDBN} reset the current PC to the beginning
> > of the function that is called. No call instruction is executed, but
> > the @code{RET} instruction actually is. To avoid a control
> > protection exception due to the missing return address on the shadow
> > stack, @value{GDBN} pushes the new return address to the shadow stack
> and updates the shadow stack pointer.
> > +@item Step:
> > +With displaced stepping, @value{GDBN} may run an out of line copy of
> > +a call instruction. In this case, the wrong return address is pushed
> > +to the shadow stack. @value{GDBN} corrects this value to avoid a
> > +control protection exception. For more details on displaced stepping, see
> @ref{displaced-stepping}.
> > +@end itemize
> >
> > @node Alpha
> > @subsection Alpha
> > @@ -41741,6 +41749,7 @@ GLOBAL Disassembler_2
> (Matches current architecture)
> > @cindex out-of-line single-stepping
> > @item set displaced-stepping
> > @itemx show displaced-stepping
> > +@anchor{displaced-stepping}
> > Control whether or not @value{GDBN} will do @dfn{displaced stepping}
> > if the target supports it. Displaced stepping is a way to single-step
> > over breakpoints without removing them from the inferior, by executing
> > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index
> > 8eb5b4fac86..3b05ace2142 100644
> > --- a/gdb/i386-tdep.c
> > +++ b/gdb/i386-tdep.c
> > @@ -899,6 +899,21 @@ i386_displaced_step_fixup (struct gdbarch
> *gdbarch,
> > displaced_debug_printf ("relocated return addr at %s to %s",
> > paddress (gdbarch, esp),
> > paddress (gdbarch, retaddr));
> > +
> > + /* If shadow stack is enabled, we need to correct the return address
> > + on the shadow stack too. */
> > + bool shadow_stack_enabled;
> > + std::optional<CORE_ADDR> ssp
> > + = gdbarch_get_shadow_stack_pointer (gdbarch, regs,
> > + shadow_stack_enabled);
> > + if (ssp.has_value () && shadow_stack_enabled)
>
> Same comment here as for amd64.

Yes, will fix.

> > + {
> > + write_memory_unsigned_integer (*ssp, retaddr_len, byte_order,
> > + retaddr);
> > + displaced_debug_printf ("relocated shadow stack return addr at %s
> "
> > + "to %s", paddress (gdbarch, *ssp),
> > + paddress (gdbarch, retaddr));
> > + }
> > }
> > }
> >
> > diff --git a/gdb/testsuite/gdb.arch/amd64-shadow-stack-disp-step.exp
> > b/gdb/testsuite/gdb.arch/amd64-shadow-stack-disp-step.exp
> > new file mode 100644
> > index 00000000000..47bb4df8cfe
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-disp-step.exp
> > @@ -0,0 +1,92 @@
> > +# 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/>.
> > +
> > +# Test continue from call instructions with shadow stack and
> > +displaced # stepping being enabled.
> > +
> > +require allow_ssp_tests support_displaced_stepping
> > +
> > +standard_testfile amd64-shadow-stack.c
> > +
> > +save_vars { ::env(GLIBC_TUNABLES) } {
> > +
> > + append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
> > +
> > + if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> > + additional_flags="-fcf-protection=return"] } {
> > + return -1
>
> All of the 'return -1' in the global scope should just be 'return'. The
> -1 is not checked or used.

Yes, will fix.

> > + }
> > +
> > + # Enable displaced stepping.
> > + gdb_test_no_output "set displaced-stepping on"
> > + gdb_test "show displaced-stepping" ".* displaced stepping .* is on.*"
> > +
> > + if { ![runto_main] } {
> > + return -1
> > + }
> > +
> > + # Get the address of the call1 instruction.
>
> I think you mean:
>
> # Get the address of the call to the call1 function.

Yes, thank you.

> > + set call1_addr -1
> > + gdb_test_multiple "disassemble main" "" {
> > + -re -wrap "($hex) <\\+($decimal)>:\\s*call\\s*0x.*<call1>.*" {
> > + set call1_addr $expect_out(1,string)
> > + pass $gdb_test_name
> > + }
> > + }
> > +
> > + if { $call1_addr == -1 } {
> > + return -1
> > + }
> > +
> > + # Get the address of the call2 instruction.
>
> As above:
>
> # Get the address of the call to the call2 function.

Will fix.

> > + set call2_addr -1
> > + gdb_test_multiple "disassemble call1" "" {
> > + -re -wrap "($hex) <\\+($decimal)>:\\s*call\\s*0x.*<call2>.*" {
> > + set call2_addr $expect_out(1,string)
> > + pass $gdb_test_name
> > + }
> > + }
> > +
> > + if { $call2_addr == -1 } {
> > + return -1
> > + }
> > +
> > + gdb_test "break *$call1_addr" \
> > + "Breakpoint $decimal at $hex.*" \
> > + "break at the address of the call1 instruction"
> > +
> > + gdb_test "break *$call2_addr" \
> > + "Breakpoint $decimal at $hex.*" \
> > + "break at the address of the call2 instruction"
> > +
> > + # Depending on instruction generation we might end up in the call
> > + # instruction after "runto_main". Only resume until call1 instruction
> > + # in case the first instruction we're stopped at is not yet the call1
> > + # instruction.
>
> Could you not just add some filler to the test program to avoid needing to
> do this? I don't really object (to this) but using filler might be simpler.

Indeed, simply adding a nop instruction will fix this:
~~~
/* Depending on instruction generation we might end up in the call
instruction of call1 function after "runto_main". Avoid this by
adding a nop instruction, to simplify the testing in
amd64-shadow-stack-disp-step.exp. */
asm ("nop");
~~~
Thank you!

> > + set stop_addr [get_valueof "/x" "\$pc" "" "value of pc after
> runto_main"]
> > + if {[eval expr "$stop_addr < $call1_addr"]} {
>
> Is the 'eval expr' really needed here? There are plenty of the places in the
> testsuite where we just write something like:
>
> if { $stop_addr < $call1_addr } { ... }
>
> so I would have expected that to work.

I see:
ERROR: tcl error sourcing /tmp/gdb.arch/amd64-shadow-stack-disp-step.exp.
ERROR: tcl error code TCL LOOKUP COMMAND {0x555555555148 < 0x000055555555514d}
ERROR: invalid command name "0x555555555148 < 0x000055555555514d"
while executing
"::gdb_tcl_unknown {0x555555555148 < 0x000055555555514d}"

I think the examples that are available in the testsuite for comparing addresses are
always inside gdb_assert.

The gdb_assert proc then again uses "expr". But I can omit "eval" at least.

Kind Regards,
Christina

> Thanks,
> Andrew
>
> > + gdb_test "continue" \
> > + "Breakpoint $decimal, $call1_addr in main ().*" \
> > + "continue until call1 instruction"
> > + }
> > + gdb_assert {$call1_addr == [get_valueof "/x" "\$pc" ""]}
> > +
> > + # Test continue from breakpoint at call1 and call2 instructions.
> > + gdb_test "continue" \
> > + "Breakpoint $decimal, $call2_addr in call1 ().*" \
> > + "continue from call1 instruction"
> > +
> > + gdb_continue_to_end "continue from call2 instruction"
> > +}
> > --
> > 2.43.0

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