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

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

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

> Until now, handling of the shadow stack pointer has been done in the
> target dependent implementations of the gdbarch hook
> 'gdbarch_shadow_stack_push'.  Also amd64 and aarch64 linux specific
> unwinders for the shadow stack pointer are implemented.
> In a following commit a subcommmand "backtrace shadow" of the ordinary

Too mmany m's in "subcommand".

> backtrace command will be added to print the shadow stack backtrace.
> This requires more target-independent logic to handle the shadow stack
> pointer.  To avoid that we duplicate the logic, add new source and header
> files "shadow-stack" for the implementation of shadow_stack_push and shadow
> stack pointer unwinding in a target-independent way.

<snip>

> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 500ac77d75a..95af82c2632 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c

As mentioned before, this change is also needed in aarch64-tdep.c to
make this patch series work for AArch64:

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 95af82c26327..9e866fc319d4 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -4780,6 +4780,10 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Register a hook for converting a memory tag to a string.  */
   set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
 
+  if (tdep->has_gcs ())
+    /* AArch64's shadow stack pointer is the GCSPR.  */
+    set_gdbarch_ssp_regnum (gdbarch, tdep->gcs_reg_base);
+
   /* ABI */
   set_gdbarch_short_bit (gdbarch, 16);
   set_gdbarch_int_bit (gdbarch, 32);

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

> -	    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¹:

> The start date should be when the patches were first posted to the list,
> or otherwise made publicly available (e.g. Intel specific GDB release?).
> The end date should be updated to 2025.

Same question for other files created by this patch series.

> +   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/>.  */
> +
> +#include "defs.h"
> +#include "arch-utils.h"
> +#include "gdbcore.h"
> +#include "extract-store-integer.h"
> +#include "frame.h"
> +#include "frame-unwind.h"
> +#include "shadow-stack.h"
> +
> +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.

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.

> +/* Return a new shadow stack pointer which is incremented or decremented
> +   by COUNT elements dependent on DIRECTION.  */
> +
> +static CORE_ADDR
> +update_shadow_stack_pointer (gdbarch *gdbarch, CORE_ADDR ssp,
> +			     const ssp_update_direction direction)
> +{
> +  bool increment = (gdbarch_stack_grows_down (gdbarch))
> +		    ? (direction == ssp_update_direction::bottom)
> +		      : (direction == ssp_update_direction::top);

All the parentheses above are superfluous and can be removed.

> +  CORE_ADDR new_ssp;

This variable is unused and can be removed. Because by default GDB build
with -Werror, this patch breaks the build with an "unused variable"
error until patch 6 which removes this variable.

> +  if (increment)
> +    return ssp + gdbarch_shadow_stack_element_size_aligned (gdbarch);
> +  else
> +    return ssp - gdbarch_shadow_stack_element_size_aligned (gdbarch);
> +}
> +
> +/* 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.

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

Also, the line above fits in 80 columns and doesn't need to be broken,
even if "&range" is changed to "nullptr".

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

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

> +}
> +
> +/* See shadow-stack.h.  */
> +
> +value *
> +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 (gdbarch_address_in_shadow_stack_memory_range_p (gdbarch)
> +      && v->entirely_available () && !v->optimized_out ())
> +    {
> +      const 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);
> +
> +      /* Only if the current shadow stack pointer SSP points to shadow
> +	 stack memory a valid previous shadow stack pointer can be
> +	 calculated.  */
> +      std::pair<CORE_ADDR, CORE_ADDR> range;
> +      if (gdbarch_address_in_shadow_stack_memory_range (gdbarch, ssp,
> +							&range))

This line fits in 80 columns and doesn't need to be broken.

> +	{
> +	  /* Note that a shadow stack memory range can change, due to
> +	     shadow stack switches for instance on x86 for an inter-
> +	     privilege far call or when calling an interrupt/exception
> +	     handler at a higher privilege level.  Shadow stack for
> +	     userspace is supported for amd64 linux starting with
> +	     Linux kernel v6.6.  However, shadow stack switches are not
> +	     supported due to missing kernel space support.  We therefore
> +	     implement this unwinder without support for shadow stack
> +	     switches for now.  */
> +	  const CORE_ADDR new_ssp
> +	    = update_shadow_stack_pointer (gdbarch, ssp,
> +					   ssp_update_direction::bottom);
> +
> +	  /* On x86, if NEW_SSP points to the end of RANGE, it indicates
> +	     that NEW_SSP is valid, but the shadow stack is empty.  In
> +	     contrast, on ARM's Guarded Control Stack, if NEW_SSP points
> +	     to the end of RANGE, it means that the shadow stack pointer
> +	     is invalid.  */
> +	  bool is_top_addr_empty_shadow_stack
> +	    = gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
> +	      && gdbarch_top_addr_empty_shadow_stack (gdbarch, new_ssp, range);
> +
> +	  /* Check whether the new SSP is valid.  Depending on the
> +	     architecture, this may rely on both
> +	     IS_TOP_ADDR_EMPTY_SHADOW_STACK and the return value of
> +	     gdbarch_address_in_shadow_stack_memory_range, or on the
> +	     latter only.  */
> +	  if (is_top_addr_empty_shadow_stack

Considering that, as previously mentioned,
gdbarch_address_in_shadow_stack_memory_range uses '<' rather than '<='
in the memory range check, AArch64 doesn't need
gdbarch_top_addr_empty_shadow_stack.

The way this if condition is written, I think it's x86_64 that would
need to provide an gdbarch_top_addr_empty_shadow_stack implementation.
I'm surprised it doesn't.

> +	      || gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> +							       ssp,

Shouldn't this argument be new_ssp?

> +							       &range))
> +	    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;
> +

Spurious empty line added here.

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

In this case, clangd is complaining here that regcache is
unknown. There's no need to include regcache.h just for it, just adding
a "class regcache" forward declaration at the beginning of the file is
enough.

> +			const CORE_ADDR new_addr);
> +
> +/* Unwind the previous shadow stack pointer of THIS_FRAME's shadow stack
> +   pointer.  REGNUM is the register number of the shadow stack pointer.
> +   Return a value that is unavailable in case we cannot unwind the
> +   previous shadow stack pointer.  Otherwise, return a value containing
> +   the previous shadow stack pointer.  */
> +
> +value * dwarf2_prev_ssp (const frame_info_ptr &this_frame,

The space after "value *" should be removed.

> +			 void **this_cache, int regnum);
> +
> +#endif /* GDB_SHADOW_STACK_H */

-- 
Thiago

¹ https://inbox.sourceware.org/gdb-patches/87ldo6c84t.fsf@redhat.com/