Re: [PATCH 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/next/x86/shstk.html.

The docs under "next/" are from the next tree, that is, patches that are
likely to be included in the next Linux version but not necessarily.

It's better to link to:

https://docs.kernel.org/arch/x86/shstk.html

which is the documentation that actually reached upstream.

> 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                        | 43 +++++++++++++++
>  .../amd64-shadow-stack-backtrace-signal.exp   | 54 +++++++++++++++++++
>  .../gdb.arch/amd64-shadow-stack-signal.c      | 31 +++++++++++
>  3 files changed, 128 insertions(+)
>  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
>
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index f0db3b7a1b4..d72525a4cab 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1952,6 +1952,46 @@ amd64_linux_get_shadow_stack_pointer (gdbarch *gdbarch, regcache *regcache,
>    return ssp;
>  }
>  
> +/* 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.  */
> +
> +static bool
> +amd64_linux_is_no_return_shadow_stack_address
> +  (gdbarch *gdbarch, const shadow_stack_frame_info &frame)
> +{
> +  /* 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));

The ssp member of a shadow_stack_frame_info object should always be in
the shadow stack memory range, no? This assert would be more effective
in that class' constructor.

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

Extraneous parentheses in the expression above.

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

> +  return (shadow_stack_val_cleared == prev_ssp);

Extraneous parentheses in the expression above.

-- 
Thiago