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

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

Hello Christina,

I was having one more look at this patch today. I just have a few nits:

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

> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index d44e59a523d..1f24a3a6d72 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -2830,9 +2830,11 @@ provided:
>    unwinding and inferior function calls
>  - top_addr_empty_shadow_stack: required for shadow stack pointer unwinding
>  - ssp_regnum: required for inferior function calls.
> -
>  If the shadow stack alignment is not the predefault of 8 bytes, configure
>  the gdbarch value shadow_stack_element_size_aligned.
> +To support the command line option 'backtrace -shadow' in addition to all
> +values and methods listed above also the gdbarch hook get_shadow_stack_size,
> +has to be provided.
>  
>  If possible, return the shadow stack pointer.  If the shadow stack
>  feature is enabled then set SHADOW_STACK_ENABLED to true, otherwise
> @@ -2893,3 +2895,19 @@ this value.
>      predefault="8",
>      invalid=False,
>  )
> +
> +Method(
> +    comment="""
> +Return the number of elements which are currently on the shadow stack
> +based on the shadow stack pointer SSP and 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.
> +""",
> +    type="long",
> +    name="get_shadow_stack_size",
> +    params=[
> +        ("const std::optional<CORE_ADDR>", "ssp"),
> +        ("const std::pair<CORE_ADDR, CORE_ADDR>", "range")
> +    ],
> +    predicate=True,
> +)

Small nit: we aim to keep the Python files in the style used by the Black
Python code formatter. If you run it on this file:

$ black gdb/gdbarch_components.py

It changes the above to:

     name="get_shadow_stack_size",
     params=[
         ("const std::optional<CORE_ADDR>", "ssp"),
-        ("const std::pair<CORE_ADDR, CORE_ADDR>", "range")
+        ("const std::pair<CORE_ADDR, CORE_ADDR>", "range"),
     ],
     predicate=True,
 )

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

"reason" can also be a reference, since it's assumed to be non-null.

> +{
> +  /* 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.  */

I like this new wording of the doc comment. Thanks!

Just one nit: UNWIND_stop_REASON should be all upper-case. :)

> +
> +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);
> +
> +  /* 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;

Just for the record: as I'm sure you realized, I was wrong in my comment
in v1's review where I said this expression to calculate level was
missing a "- 1" term. This looks good. Sorry about that.

I think this function and the loop that calls it in
backtrace_shadow_command are all good now.

> +  /* 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});
> +}
> +
> +std::optional<shadow_stack_frame_info>
> +shadow_stack_frame_info::unwind_prev_shadow_stack_frame_info
> +  (gdbarch *gdbarch, std::pair<CORE_ADDR, CORE_ADDR> range)
> +{
> +  /* If the user's backtrace limit has been exceeded, stop.  We must
> +     add two to the current level; one of those accounts for
> +     backtrace_limit being 1-based and the level being 0-based, and the
> +     other accounts for the level of the new frame instead of the level
> +     of the current frame.  */
> +  if (this->level + 2 > user_set_backtrace_options.backtrace_limit)
> +    return {};
> +
> +  CORE_ADDR new_ssp
> +    = update_shadow_stack_pointer (gdbarch, this->ssp, 1,
> +				   ssp_update_direction::outer);
> +
> +   if (gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
> +       && gdbarch_top_addr_empty_shadow_stack (gdbarch, new_ssp, range))
> +     return {};
> +   else if (gdbarch_stack_grows_down (gdbarch))

The four lines above are indented with one extra space.

⋮
> +  if (read_shadow_stack_memory (gdbarch, *start_ssp, new_value, &reason))
> +    current = {*start_ssp, new_value, 0, ssp_unwind_stop_reason::no_error};
> +
> +  std::optional<shadow_stack_frame_info> trailing = current;
> +
> +  LONGEST count = -1;
> +  if (current.has_value () && count_exp != nullptr)
> +    {
> +      count = parse_and_eval_long (count_exp);
> +      /* If count is negative, update trailing with the shadow stack frame
> +	 info from which we should start printing.  */
> +      if (count < 0)
> +	{
> +	  trailing = get_trailing_outermost_shadow_stack_frame_info
> +		       (gdbarch, range, std::abs (count), *current);
> +
> +	  if (!trailing.has_value ())
> +	    reason = current->unwind_stop_reason;
> +	}
> +    }
> +
> +  if (!trailing.has_value ())
> +    {
> +      gdb_assert (reason != ssp_unwind_stop_reason::no_error);
> +
> +      error (_("Cannot print shadow stack backtrace: %s.\n"),
> +	     ssp_unwind_stop_reason_to_err_string (reason));
> +    }
> +
> +  current = trailing;

The assignment above is now redundant, since it's also in the for loop
initial condition.

> +  for (current = trailing; current.has_value () && count != 0; count--)
> +    {
> +      QUIT;
> +
> +      print_shadow_stack_frame_info (gdbarch, fp_opts, *current, LOCATION);
> +
> +      trailing = current;
> +      current = current->unwind_prev_shadow_stack_frame_info (gdbarch, range);
> +    }
> +
> +  /* If we've stopped before the end, mention that.  */
> +  if (current.has_value () && from_tty)
> +    gdb_printf (_("(More shadow stack frames follow...)\n"));
> +
> +  /* Due to the loop above, trailing always has a value at this point.  */
> +  gdb_assert (trailing.has_value ());
> +
> +  /* If we've run out of shadow stack frames, and the reason appears to
> +     be an error condition, print it.  */
> +  if (!current.has_value ()
> +      && trailing->unwind_stop_reason > ssp_unwind_stop_reason::no_error)
> +    gdb_printf (_("Shadow stack backtrace stopped at shadow stack " \
> +		  "pointer %s due to: %s.\n"),
> +		paddress (gdbarch, trailing->ssp),
> +		ssp_unwind_stop_reason_to_err_string
> +		  (trailing->unwind_stop_reason));
> +}

-- 
Thiago