Re: [PATCH v2 8/9] 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:

> There can be elements on the shadow stack which are not return addresses.
> This can happen, for instance, in case of signals on amd64 linux.
> The old shadow stack pointer is pushed in a special format with bit 63 set.
>
> |1...old SSP| - Pointer to old pre-signal ssp in sigframe token format
>                 (bit 63 set to 1)
>
> Linux kernel documentation: https://docs.kernel.org/arch/x86/shstk.html
>
> Implement the gdbarch hook is_no_return_shadow_stack_address to detect
> this scenario to print the shadow stack backtrace correctly.
> ---
>  gdb/amd64-linux-tdep.c                        | 55 ++++++++++++++++++-
>  .../amd64-shadow-stack-backtrace-signal.exp   | 49 +++++++++++++++++
>  .../gdb.arch/amd64-shadow-stack-signal.c      | 31 +++++++++++
>  3 files changed, 134 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-backtrace-signal.exp
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-signal.c

Just one nit below. In any case:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

I don't know much about MI, so I won't review patch 9.

> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index a4eabccf667..b517f9772f5 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1986,6 +1986,56 @@ amd64_linux_get_shadow_stack_size
>    return shadow_stack_bytes / 8;
>  }
>  
> +/* Return true, if FRAME is a valid shadow stack frame while FRAME.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, configure the string
> +   which describes the frame and is displayed instead of the address in
> +   the shadow stack backtrace.  */
> +
> +static bool
> +amd64_linux_is_no_return_shadow_stack_address
> +  (gdbarch *gdbarch,
> +   const shadow_stack_frame_info &frame,
> +   std::string &frame_type)
> +{
> +  /* FRAME must be a valid shadow stack frame.  */
> +  std::pair<CORE_ADDR, CORE_ADDR> range;
> +  gdb_assert (gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> +							    frame.ssp,
> +							    &range));
> +
> +  /* 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 ((frame.value & mask) == 0)
> +    return false;
> +
> +  /* 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 = (frame.value & (~mask));
> +
> +  /* Compute the previous/old SSP.  The shadow stack grows downwards.  To
> +     compute the previous shadow stack pointer, we need to increment
> +     FRAME.SSP.  */
> +  CORE_ADDR prev_ssp
> +    = frame.ssp + gdbarch_shadow_stack_element_size_aligned (gdbarch);
> +
> +  /* We incremented FRAME.SSP by one element to compute PREV_SSP before.
> +     In case FRAME.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);

It's better to use one gdb_assert per condition, so that if it triggers,
it's clear which condition was violated.

> +
> +  if (shadow_stack_val_cleared == prev_ssp)
> +    {
> +      frame_type = _("<sigframe token>");
> +      return true;
> +    }
> +
> +  return false;
> +}

-- 
Thiago