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

[Luis Machado <luis.machado@arm.com>]

On 6/23/25 19:24, Schimpe, Christina wrote:
> 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.
>  

That makes it better. Thanks!

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

Alright. This is OK then.

>>
>>> +
>>>  * 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, sorry. I messed that one up.

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

I think it is enough to mention that depending on instruction generation we might end up
in the call instruction after "runto_main". Then we don't make any references to versions,
since they change in behavior sometimes.


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