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

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

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