Re: [PATCH 1/9] gdb: Generalize handling of the shadow stack pointer.

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Christina,

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> Thanks a lot for this detailed review!
> I applied most of your comments, please find my feedback to your review below.

You're welcome!

>> -----Original Message-----
>> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Sent: Friday, 31 October 2025 02:32
>> To: Schimpe, Christina <christina.schimpe@intel.com>
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 1/9] gdb: Generalize handling of the shadow stack
>> pointer.
>> 
>> Christina Schimpe <christina.schimpe@intel.com> writes:
>> 
>> > -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);
>> > -
>> > -      /* 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);
>> > -
>> > -	  /* 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).  */
>> > -	  if (new_ssp <= range.second)
>> 
>> IIUC, the '<=' comparison above isn't preserved by this patch. This function is
>> replaced by dwarf2_prev_ssp, which uses
>> gdbarch_address_in_shadow_stack_memory_range for this if condition,
>> whose comparison in find_addr_mem_range is:
>> 
>>       bool addr_in_mem_range
>>         = (addr >= map.start_address && addr < map.end_address);
>> 
>> Is this intended?
>
> Arg, thanks for catching that!
>
> I think I missed that because I introduced a typo/bug in the call
>
> 	      || gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> 							       ssp,
> 							       &range))
>
> which made the unwinding work properly in case of amd64.
> However, the proper fix should be to pass new_ssp to gdbarch_address_in_shadow_stack_memory_range
> instead, and to implement gdbarch_top_addr_empty_shadow_stack also for amd64.
>
> Does that make sense?

Yes, I agree.

>> > -	    return frame_unwind_got_address (this_frame, regnum, new_ssp);
>> > -	}
>> > -    }
>> > -
>> > -  /* Return a value which is marked as unavailable in case we could not
>> > -     calculate a valid previous shadow stack pointer.  */
>> > -  value *retval
>> > -    = value::allocate_register (get_next_frame_sentinel_okay (this_frame),
>> > -				regnum, register_type (gdbarch, regnum));
>> > -  retval->mark_bytes_unavailable (0, retval->type ()->length ());
>> > -  return retval;
>> > -}
>> 
>> <snip>
>> 
>> > diff --git a/gdb/shadow-stack.c b/gdb/shadow-stack.c new file mode
>> > 100644 index 00000000000..d153d5fc846
>> > --- /dev/null
>> > +++ b/gdb/shadow-stack.c
>> > @@ -0,0 +1,167 @@
>> > +/* Manage a shadow stack pointer for GDB, the GNU debugger.
>> > +
>> > +   Copyright (C) 2024-2025 Free Software Foundation, Inc.
>> 
>> Should this really start at 2024? According to Andrew Burgess¹:
>
> Yes, 2024 is correct in this case since our gdb-oneapi supported bt shadow since 2024. 

Ah, right. Thanks for clarifying.

>> > +enum class ssp_update_direction
>> > +{
>> > +  /* Update ssp towards the bottom of the shadow stack.  */
>> > +  bottom = 0,
>> > +
>> > +  /* Update ssp towards the top of the shadow stack.  */
>> > +  top
>> > +};
>> 
>> I find the bottom/top nomenclature confusing, especially because it's
>> supposed to mean the same thing whether the stack grows up or down. In
>> my mind, if the stack grow down then top means "oldest element", but if the
>> stack grows up, then top means "newest element".
>> But in this patch it seems that top means "newest element" regardless of the
>> direction of stack growth.
>
> Yes, that was my understanding. So independent in which direction a shadow stack
> grows based on the architecture/OS, top always means newest element.  But I think
> it is not a problem to take one of your suggestions.

Thank you. In my view it also matches the nomenclature in frame.h, which
also doesn't use vertical concepts. E.g.,

  /* Given a FRAME, return the next (more inner, younger) or previous
     (more outer, older) frame.  */
  extern frame_info_ptr get_prev_frame (const frame_info_ptr &);
  extern frame_info_ptr get_next_frame (const frame_info_ptr &);

>> I would suggest changing the enum names above to something that's not
>> related to the vertical axis, so that their meaning will be clear regardless of
>> which direction the stack grows. A few suggestions:
>> shrink/grow, older/younger, outer/inner.
>
> I'd take outer/inner and describe it as follows:
>
> enum class ssp_update_direction
> {
>   /* Update ssp towards the oldest (outermost) element of the shadow
>      stack.  */
>   outer = 0,
>
>   /* Update ssp towards the most recent (innermost) element of the
>      shadow stack.  */
>   inner
> };
>
> Is that understandable ?

Yes, thanks for making the change.

>> > +/* See shadow-stack.h.  */
>> > +
>> > +void shadow_stack_push (gdbarch *gdbarch, regcache *regcache,
>> 
>> There's no need for a gdbarch argument. You can get it from the regcache.
>
> Fixed.
>
>> > +			const CORE_ADDR new_addr)
>> > +{
>> > +  if (!gdbarch_address_in_shadow_stack_memory_range_p (gdbarch)
>> > +      || gdbarch_ssp_regnum (gdbarch) == -1)
>> > +    return;
>> > +
>> > +  bool shadow_stack_enabled;
>> > +  std::optional<CORE_ADDR> ssp
>> > +    = gdbarch_get_shadow_stack_pointer (gdbarch, regcache,
>> > +					shadow_stack_enabled);
>> > +  if (!ssp.has_value () || !shadow_stack_enabled)
>> > +    return;
>> > +
>> > +  const CORE_ADDR new_ssp
>> > +    = update_shadow_stack_pointer (gdbarch, *ssp,
>> > +				   ssp_update_direction::top);
>> > +
>> > +  /* If NEW_SSP does not point to shadow stack memory, we assume the stack
>> > +     is full.  */
>> > +  std::pair<CORE_ADDR, CORE_ADDR> range;
>> > +  if (!gdbarch_address_in_shadow_stack_memory_range (gdbarch,
>> > +						     new_ssp,
>> > +						     &range))
>> 
>> Range isn't really needed by this function. I suggest changing
>> gdbarch_address_in_shadow_stack_memory_range to allow for it to be
>> nullptr and then pass nullptr here.
>
> I agree, fixed.
>
>> Also, the line above fits in 80 columns and doesn't need to be broken, even if
>> "&range" is changed to "nullptr".
>
> It is more than 80 columns for me.

Hm. When I edited it here and changed "&range" to "nullptr" the line
ended exactly at column 80. Which is arguably not ideal, so I don't mind
either way.

>> > +    error (_("No space left on the shadow stack."));
>> > +
>> > +  /* On x86 there can be a shadow stack token at bit 63.  For x32,  the
>> > +     address size is only 32 bit.  Always write back the full 8 bytes to
>> > +     include the shadow stack token.  */
>> 
>> s/8 bytes/element size/
>
> Fixed.
>
>> 
>> > +  const int element_size
>> > +    = gdbarch_shadow_stack_element_size_aligned (gdbarch);
>> > +
>> > +  const bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> > +
>> > +  write_memory_unsigned_integer (new_ssp, element_size, byte_order,
>> > +				 (ULONGEST) new_addr);
>> > +
>> > +  regcache_raw_write_unsigned (regcache,
>> > +			       gdbarch_ssp_regnum (gdbarch),
>> > +			       new_ssp);
>> 
>> The line above fits in 80 columns and doesn't need to be broken.
>
> I count 81 columns and there is also a soft limit of 74 characters:
>
> https://sourceware.org/legacy-ml/gdb-patches/2014-01/msg00216.html

Ah, I wasn't aware of the soft limit. Thanks for pointing it out.

> So I'll keep it as is, if that's fine for you.

Yes, of course.

>> > diff --git a/gdb/shadow-stack.h b/gdb/shadow-stack.h new file mode
>> > 100644 index 00000000000..5c3ba80974e
>> > --- /dev/null
>> > +++ b/gdb/shadow-stack.h
>> > @@ -0,0 +1,39 @@
>> > +/* Definitions to manage a shadow stack pointer for GDB, the GNU debugger.
>> > +
>> > +   Copyright (C) 2024-2025 Free Software Foundation, Inc.
>> > +
>> > +   This file is part of GDB.
>> > +
>> > +   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/>.  */
>> > +
>> > +#ifndef GDB_SHADOW_STACK_H
>> > +#define GDB_SHADOW_STACK_H
>> > +
>> > +/* If shadow stack is enabled, push the address NEW_ADDR on the shadow
>> > +   stack and update the shadow stack pointer accordingly.  */
>> > +
>> > +void shadow_stack_push (gdbarch *gdbarch, regcache *regcache,
>> 
>> Recently, the project has been trying to make the header files contain all the
>> headers and definitions that they need, for the benefit of IDE and language
>> server users, so that these tools don't emit spurious errors when showing a
>> header file.
>
> Ah, ok I wasn't aware. Do you have a link for that ? I think I cannot follow 100 %.

There was a discussion about it in this thread:

https://sourceware.org/pipermail/gdb-patches/2024-February/206632.html

It resulted in this patch:

https://inbox.sourceware.org/gdb-patches/20240326190806.89541-4-simon.marchi@efficios.com/

And it's also in the wiki²:

  A .c, .cc or .h file should directly include the .h file of every
  declaration and/or definition it directly refers to. Exception: Do not
  include defs.h, server.h, common-defs.h directly.

-- 
Thiago

² https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Include_Files