Re: [PATCH v2 6/9] gdb: Add command option 'bt -shadow' to print the shadow stack backtrace.

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

I'll have a closer look at this patch next week, including your comments
about the count argument to the
get_trailing_outermost_shadow_stack_frame_info in the thread for v1 of
this series.. Unfortunately I didn't have much time to dig into it yet.

Some minor comments for now:

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

> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 656daa0f0ee..a4eabccf667 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1963,6 +1963,29 @@ amd64_linux_top_addr_empty_shadow_stack
>    return addr == range.second;
>  }
>  
> +/* Return the number of elements which are currently on the shadow stack
> +   based on the shadow stack memory RANGE [start_address, end_address)
> +   of the current thread.  In case shadow stack is not enabled for the
> +   current thread, return -1.  */
> +
> +static long
> +amd64_linux_get_shadow_stack_size
> +  (gdbarch *gdbarch,
> +   const std::optional<CORE_ADDR> ssp,
> +   const std::pair<CORE_ADDR, CORE_ADDR> range)
> +{
> +  /* For x86, if we don't have a shadow stack pointer, we can assume
> +     that the shadow stack is disabled for the current thread.  */
> +  if (!ssp.has_value ())
> +    return -1;

As I mention a bit below, the only caller of this function passes a
value for the ssp argument, so this is dead code.

> +  const unsigned long shadow_stack_bytes = range.second - *ssp;
> +
> +  gdb_assert ((shadow_stack_bytes % 8) == 0);

I don't think this should be an assert. If it fails, it triggers an
internal error in GDB.  In this case it could indeed mean an internal
error (GDB somehow got the SSP or range wrong), but it could also be
(and probably more likely) an inconsistent state of the inferior. This
can happen in a program being debugged so GDB should be able to handle
it gracefully, and if possible provide useful information to the user.

> +  return shadow_stack_bytes / 8;
> +}

  ⋮

> +/* Read the memory at shadow stack pointer SSP and assign it to
> +   RETURN_VALUE.  In case we cannot read the memory, set REASON to
> +   ssp_unwind_stop_reason::memory_read_error and return false.  */
> +
> +static bool
> +read_shadow_stack_memory (gdbarch *gdbarch, CORE_ADDR ssp,
> +			  CORE_ADDR &return_value,
> +			  ssp_unwind_stop_reason *reason)

The reason argument can also be a reference.

> +{
> +  /* On x86 there can be a shadow stack token at bit 63.  For x32, the
> +     address size is only 32 bit.  Thus, we still must use
> +     gdbarch_shadow_stack_element_size_aligned (and not gdbarch_addr_bit)
> +     to read the full element for x32 as well.  */
> +  const int element_size
> +    = gdbarch_shadow_stack_element_size_aligned (gdbarch);
> +
> +  const bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  if (!safe_read_memory_unsigned_integer (ssp, element_size, byte_order,
> +					  &return_value))
> +    {
> +      *reason = ssp_unwind_stop_reason::memory_read_error;
> +      return false;
> +    }
> +
> +  return true;
> +}
> +
> +/*  If possible, return the starting shadow stack frame info needed to handle
> +    COUNT outermost frames.  FRAME should point to the innermost (newest)
> +    element of the shadow stack.  RANGE is the shadow stack memory range
> +    [start_address, end_address) corresponding to FRAME's shadow stack pointer.
> +    If COUNT is bigger than the number of elements on the shadow stack, return
> +    FRAME.  In case of failure, assign an appropriate ssp_unwind_stop_reason in
> +    FRAME->UNWIND_stop_REASON.  */
> +
> +static std::optional<shadow_stack_frame_info>
> +get_trailing_outermost_shadow_stack_frame_info
> +  (gdbarch *gdbarch, const std::pair<CORE_ADDR, CORE_ADDR> range,
> +   const ULONGEST count, shadow_stack_frame_info &frame)
> +{
> +  gdb_assert (gdbarch_get_shadow_stack_size_p (gdbarch));
> +
> +  const long shadow_stack_size
> +    = gdbarch_get_shadow_stack_size (gdbarch,
> +				     std::optional<CORE_ADDR> (frame.ssp),
> +				     range);

This is the only caller of gdbarch_get_shadow_stack_size. Does its ssp
argument need to be std::optional<CORE_ADDR>, or can it simply be a
CORE_ADDR?

> +  /* We should only get here in case shadow stack is enabled for the
> +     current thread.  */
> +  gdb_assert (shadow_stack_size >= 0);
> +
> +  const long level = shadow_stack_size - count;
> +
> +  /* COUNT exceeds the number of elements on the shadow stack.  Return the
> +     starting shadow stack frame info FRAME.  */
> +  if (level <= 0)
> +    return std::optional<shadow_stack_frame_info> (frame);
> +
> +  CORE_ADDR new_ssp = update_shadow_stack_pointer
> +    (gdbarch, frame.ssp, level, ssp_update_direction::outer);
> +
> +  if (gdbarch_stack_grows_down (gdbarch))
> +    gdb_assert (new_ssp < range.second);
> +  else
> +    gdb_assert (new_ssp >= range.first);
> +
> +  CORE_ADDR new_value;
> +  if (!read_shadow_stack_memory (gdbarch, new_ssp, new_value,
> +				 &frame.unwind_stop_reason))
> +    return {};
> +
> +  return std::optional<shadow_stack_frame_info>
> +    ({new_ssp, new_value, (unsigned long) level,
> +      ssp_unwind_stop_reason::no_error});
> +}

  ⋮

> diff --git a/gdb/shadow-stack.h b/gdb/shadow-stack.h
> index 5f8395ec047..5370becfc9a 100644
> --- a/gdb/shadow-stack.h
> +++ b/gdb/shadow-stack.h
> @@ -35,4 +35,10 @@ void shadow_stack_push (regcache *regcache, const CORE_ADDR new_addr);
>  value *dwarf2_prev_ssp (const frame_info_ptr &this_frame,
>  			void **this_cache, int regnum);
>  
> +/* Implementation of "backtrace shadow" comand.  */
> +
> +void backtrace_shadow_command
> +  (const frame_print_options &fp_opts,
> +   const char *count_exp, int from_tty);
> +
>  #endif /* GDB_SHADOW_STACK_H */

This header needs to forward-declare "struct frame_print_options;",
similarly to the "class regcache;" forward declaration.

Actually, one thing I missed in my review of patch 1 is that it also
needs to forward-declare "class frame_info_ptr;" which is used in the
prototype of dwarf2_prev_ssp.

-- 
Thiago