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

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

Hi Luis,

Please find my comments below.

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Thursday, June 19, 2025 11:27 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; eliz@gnu.org
> Subject: Re: [PATCH v4 11/11] gdb: Enable displaced stepping with shadow stack
> on amd64 linux.
> 
> On 6/17/25 13:11, Christina Schimpe wrote:
> > This patch enables displaced stepping to support Intel's Control-Flow
> > Enforcement Technology (CET), which provides the shadow stack feature
> > for the x86 architecture.
> > Following the restriction of the linux kernel, enable displaced
> > stepping for amd64 only.
> >
> > 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.
> 
> I thought the description was slightly confusing. The above is the original
> behavior, right?

Yes.

> >
> > To avoid a control-protection exception if shadow stack is active, the
> > shadow stack top address must be corrected as well.
> 
> And this one is the new behavior?
> 
> Might make sense to clarify it.

I agree, does this sound better?

[...], enable displaced stepping for amd64 only.

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: 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 | 90
> > +++++++++++++++++++
> >  6 files changed, 147 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 b8fb7f0e484..15d2772230e 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.
> 
> Should we mention CET somewhere?

Hm, in theory, this should work for AMD64 in general. But I don't know about
the state of AMD's shadow stack and am not sure how I could explain this in the
NEWS properly.

> 
> > +
> >  * 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
> > d847248659a..f989cfb3bf8 100644
> > --- a/gdb/amd64-linux-tdep.c
> > +++ b/gdb/amd64-linux-tdep.c
> > @@ -1935,8 +1935,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) @@ -1954,6 +1956,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;
> >  }
> >
> > @@ -1964,8 +1969,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);
> > +
> > +  /* It's enough to check if SSP is valid as for amd64 linux shadow stack
> > +     is always enabled if SSP has a value.  */
> 
> Is my understanding correct that for amd64's shadow stack support, whenever
> SSP has a value, then shadow stack is enabled?

Yes, exactly.

> 
> If so, maybe rephrase it as...
> 
> "For amd64/Linux, if SSP has a value that means shadow stack is enabled."
> 
> What do you think?

Yes, this makes it clearer. Thank you.

> >    if (!ssp.has_value ())
> >      return;
> >
> > @@ -2121,6 +2131,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
> > 79f7e427841..6c54957ae75 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)
> > +	{
> > +	  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
> > cf152bd1e6f..589fd50345f 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -27055,12 +27055,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
> > +on the shadow
> 
> s/pushed on/pushed to

Will fix.

> 
> > +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
> > @@ -41736,6 +41744,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
> > f3fa4e511e6..d83fdc0c85e 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)
> > +	{
> > +	  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..b5f168c2c42
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-disp-step.exp
> > @@ -0,0 +1,90 @@
> > +# Copyright 2024 Free Software Foundation, Inc.
> 
> s/2024/2025

Will fix.

> > +
> > +# 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
> > +    }
> > +
> > +    # 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.
> > +    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.
> > +    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"
> > +
> > +    # We only resume until call1 instruction in case the first instruction
> > +    # we're stopped at is not yet the call1 instruction.
> > +    set stop_addr [get_valueof "/x" "\$pc" "" "value of pc after runto_main"]
> > +    if {[eval expr "$stop_addr < $call1_addr"]} {
> > +	gdb_test "continue" \
> > +	    "Breakpoint $decimal, $call1_addr in main ().*" \
> > +	    "continue until call1 instruction"
> > +    }
> 
> It was particularly clear why we need the check above. Is this due to how the
> compiler might generate code and then we could risk stopping at the
> instruction we're interested in when we "run to main"?

I assume you mean "It was not clear" in 
"It was particularly clear why we need the check above." :

Yes, this is related to compiler behaviour.
With gcc 15 the assembler changed a bit and with "run to main" we stopped already
at the call instruction.

I agree, the comment might be confusing.  Would such a comment be better?

# For gcc 15 we already stop at the call instruction when we "run to "main".
# Only resume in case the first instruction we're stopped at is not yet the call
# 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"
> > +}
> 
> Reviewed-By: Luis Machado <luis.machado@arm.com>

Thanks for all the review efforts!
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