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

[Andrew Burgess <aburgess@redhat.com>]

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

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

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

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

> +    }
> +
> +    # 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.

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

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

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

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