Re: [PATCH v4 07/11] gdb: Handle shadow stack pointer register unwinding for amd64 linux.

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

On 6/23/25 16:00, Schimpe, Christina wrote:
> Hi Luis, 
> 
> Thanks for the feedback. Please find my comments below.
> 
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: Thursday, June 19, 2025 11:25 AM
>> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
>> patches@sourceware.org
>> Cc: thiago.bauermann@linaro.org; eliz@gnu.org
>> Subject: Re: [PATCH v4 07/11] gdb: Handle shadow stack pointer register
>> unwinding for amd64 linux.
>>
>> On 6/17/25 13:11, Christina Schimpe wrote:
>>> Unwind the $pl3_ssp register.
>>> We now have an updated value for the shadow stack pointer when moving
>>> up or down the frame level.  Note that $pl3_ssp can become unavailable
>>> when moving to a frame before the shadow stack enablement.  In the
>>> example below, shadow stack is enabled in the function 'call1'.  Thus,
>>> when moving to a frame level above the function, $pl3_ssp will become
>>> unavaiable.
>>> Following the restriction of the linux kernel, implement the unwinding
>>> for amd64 linux only.
>>>
>>> Before this patch:
>>> ~~~
>>> Breakpoint 1, call2 (j=3) at sample.c:44
>>> 44	  return 42;
>>> (gdb) p $pl3_ssp
>>> $1 = (void *) 0x7ffff79ffff8
>>> (gdb) up
>>> 55	  call2 (3);
>>> (gdb) p $pl3_ssp
>>> $2 = (void *) 0x7ffff79ffff8
>>> (gdb) up
>>> 68	  call1 (43);
>>> (gdb) p $pl3_ssp
>>> $3 = (void *) 0x7ffff79ffff8
>>> ~~~
>>>
>>> After this patch:
>>> ~~~
>>> Breakpoint 1, call2 (j=3) at sample.c:44
>>> 44	  return 42;
>>> (gdb) p $pl3_ssp
>>> $1 = (void *) 0x7ffff79ffff8
>>> (gdb) up
>>> 55	  call2 (3);
>>> (gdb) p $pl3_ssp
>>> $2 = (void *) 0x7ffff7a00000
>>> (gdb) up
>>> 68	  call1 (43i);
>>> (gdb) p $pl3_ssp
>>> $3 = <unavailable>
>>> ~~~
>>>
>>> As we now have an updated value for each selected frame, the return
>>> command is now enabled for shadow stack enabled programs, too.
>>>
>>> We therefore add a test for the return command and shadow stack
>>> support, and for an updated shadow stack pointer after a frame level change.
>>>
>>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>>> ---
>>>  gdb/amd64-linux-tdep.c                        | 84 ++++++++++++++++++
>>>  gdb/linux-tdep.c                              | 47 ++++++++++
>>>  gdb/linux-tdep.h                              |  7 ++
>>>  .../gdb.arch/amd64-shadow-stack-cmds.exp      | 88 +++++++++++++++++++
>>>  gdb/testsuite/gdb.arch/amd64-shadow-stack.c   | 13 +++
>>>  5 files changed, 239 insertions(+)
>>>  create mode 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
>>>
>>> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index
>>> d806d3cb1f7..9436f0b190c 100644
>>> --- a/gdb/amd64-linux-tdep.c
>>> +++ b/gdb/amd64-linux-tdep.c
>>> @@ -47,6 +47,8 @@
>>>  #include "arch/amd64-linux-tdesc.h"
>>>  #include "inferior.h"
>>>  #include "x86-tdep.h"
>>> +#include "dwarf2/frame.h"
>>> +#include "frame-unwind.h"
>>>
>>>  /* The syscall's XML filename for i386.  */  #define
>>> XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
>>> @@ -1917,6 +1919,87 @@ amd64_linux_get_tls_dtv_addr (struct gdbarch
>> *gdbarch, ptid_t ptid,
>>>    return dtv_addr;
>>>  }
>>>
>>> +/* Return the number of bytes required to update the shadow stack pointer
>>> +   by one element.  For x32 the shadow stack elements are still 64-bit
>>> +   aligned.  Thus, gdbarch_addr_bit cannot be used to compute the new
>>> +   stack pointer.  */
>>> +
>>> +static inline int
>>> +amd64_linux_shadow_stack_element_size_aligned (gdbarch *gdbarch) {
>>> +  const bfd_arch_info *binfo = gdbarch_bfd_arch_info (gdbarch);
>>> +  return (binfo->bits_per_word / binfo->bits_per_byte); }
>>> +
>>> +
>>> +/* Implement shadow stack pointer unwinding. For each new shadow stack
>>> +   pointer check if its address is still in the shadow stack memory range.
>>> +   If it's outside the range set the returned value to unavailable,
>>> +   otherwise return a value containing the new shadow stack pointer.
>>> +*/
>>> +
>>> +static value *
>>> +amd64_linux_dwarf2_prev_ssp (const frame_info_ptr &this_frame,
>>> +			     void **this_cache, int regnum) {
>>> +  value *v = frame_unwind_got_register (this_frame, regnum, regnum);
>>> +  gdb_assert (v != nullptr);
>>> +
>>> +  gdbarch *gdbarch = get_frame_arch (this_frame);
>>> +
>>> +  if (v->entirely_available () && !v->optimized_out ())
>>> +    {
>>> +      int size = register_size (gdbarch, regnum);
>>> +      bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> +      CORE_ADDR ssp = extract_unsigned_integer (v->contents_all ().data (),
>>> +						size, byte_order);
>>> +
>>> +      /* Starting with v6.6., the Linux kernel supports CET shadow stack.
>>
>> Same typo as before, period after "v6.6".
> 
> Will fix.
> 
>>
>>> +	 Using /proc/PID/smaps we can only check if the current shadow
>>> +	 stack pointer SSP points to shadow stack memory.  Only if this is
>>> +	 the case a valid previous shadow stack pointer can be
>>> +	 calculated.  */
>>> +      std::pair<CORE_ADDR, CORE_ADDR> range;
>>> +      if (linux_address_in_shadow_stack_mem_range (ssp, &range))
>>> +	{
>>> +	  /* The shadow stack grows downwards.  To compute the previous
>>> +	     shadow stack pointer, we need to increment SSP.  */
>>> +	  CORE_ADDR new_ssp
>>> +	    = ssp + amd64_linux_shadow_stack_element_size_aligned (gdbarch);
>>> +
>>> +	  /* If NEW_SSP points to the end of or before (<=) the current
>>> +	     shadow stack memory range we consider NEW_SSP as valid (but
>>> +	     empty).  */
>>
>> I couldn't quite understand the difference between the empty case and the
>> unavailable case. But maybe I just don't fully understand the feature.
>>
>> Would it be possible to make the comment a bit more clear?
> 
> Is this a bit clearer?
> 
> "There can be scenarios where we have a shadow stack pointer but the shadow stack
> is empty, as no call instruction has been executed yet.  If NEW_SSP points to the end
> of or before (<=) the current shadow stack memory range we consider NEW_SSP as
> valid (but empty). "

Yes, that clear it up. Thanks!

> 
> Please also see my answer to Thiago:
> https://sourceware.org/pipermail/gdb-patches/2025-June/218908.html

Thanks. It would be nice if both series were consistent in this regard (<= or <).