Re: [PATCH v4 10/13] gdb: Implement the hook 'is_no_return_shadow_stack_address' for amd64 linux.

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

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

> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 42a62eaa975..d98415b8989 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1960,6 +1960,62 @@ amd64_linux_top_addr_empty_shadow_stack
>    return addr == range.second;
>  }
>  
> +/* Return a shadow stack frame info, if the shadow stack pointer SSP
> +   belongs to a valid shadow stack frame while the element on the shadow
> +   stack VALUE does not refer to a return address.  This can happen, for
> +   instance, in case of signals.  The old shadow stack pointer is pushed
> +   in a special format with bit 63 set.  In case this is true, a valid
> +   shadow stack frame info is returned with its attributes frame_type and
> +   non_return_description configured to ssp_frame_type::non_return_frame
> +   and "<sigframe token>", respectively.  */
> +
> +static std::optional<shadow_stack_frame_info>
> +amd64_linux_is_no_return_shadow_stack_address
> +  (gdbarch *gdbarch, const CORE_ADDR ssp, const CORE_ADDR value,
> +   const unsigned long level)
> +{
> +  /* SSP must belong to the shadow stack memory range.  */
> +  std::pair<CORE_ADDR, CORE_ADDR> range;
> +  gdb_assert (gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> +							    ssp,
> +							    &range));

I made this comment at another assert in a previous version of this
series, but it's valid here too:

Won't this make GDB crash with a misbehaving inferior? 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.

Maybe turn this into an error ()?

> +
> +  /* In case bit 63 is not configured, the address on the shadow stack
> +     should be a return address.  */
> +  constexpr CORE_ADDR mask = (CORE_ADDR) 1 << 63;
> +  if ((value & mask) == 0)
> +    return {};
> +
> +  /* To compare the shadow stack pointer of the previous frame with the
> +     value of FRAME, we must clear bit 63.  */
> +  CORE_ADDR shadow_stack_val_cleared = (value & (~mask));
> +
> +  /* Compute the previous/old SSP.  The shadow stack grows downwards.  To
> +     compute the previous shadow stack pointer, we need to increment
> +     SSP.  */
> +  CORE_ADDR prev_ssp
> +    = ssp + gdbarch_shadow_stack_element_size_aligned (gdbarch);
> +
> +  /* We incremented SSP by one element to compute PREV_SSP before.  In
> +     case SSP points to the first element of the shadow stack, PREV_SSP
> +     must point to the bottom of the shadow stack (RANGE.SECOND), but not
> +     beyond that address.  */
> +  gdb_assert (prev_ssp > range.first && prev_ssp <= range.second);
> +
> +  if (shadow_stack_val_cleared == prev_ssp)
> +    {
> +      /* Assign the current gdbarch to the new shadow stack frame.  Since
> +	 the token is no PC value, do not assign a SAL object.  */
> +      return std::optional<shadow_stack_frame_info>
> +	({ssp, value, level, gdbarch, {},
> +	  ssp_frame_type::non_return_frame,
> +	  {"<sigframe token>"},
> +	  ssp_unwind_stop_reason::no_error});
> +    }
> +
> +  return {};
> +}
> +

-- 
Thiago
(he/him)