Re: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method to get the shadow stack pointer.

[Andrew Burgess <aburgess@redhat.com>]

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

> This patch is required by the following commit
> "gdb: Enable displaced stepping with shadow stack on amd64 linux."
>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Reviewed-By: Luis Machado <luis.machado@arm.com>
> ---
>  gdb/arch-utils.c          | 10 ++++++++++
>  gdb/arch-utils.h          |  5 +++++
>  gdb/gdbarch-gen.c         | 22 ++++++++++++++++++++++
>  gdb/gdbarch-gen.h         | 12 +++++++++++-
>  gdb/gdbarch_components.py | 17 ++++++++++++++++-
>  5 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index f320d3d7365..c396e9e3840 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1218,6 +1218,16 @@ default_gdbarch_return_value
>  				readbuf, writebuf);
>  }
>  
> +/* See arch-utils.h.  */
> +
> +std::optional<CORE_ADDR>
> +default_get_shadow_stack_pointer (gdbarch *gdbarch, regcache *regcache,
> +				  bool &shadow_stack_enabled)
> +{
> +  shadow_stack_enabled = false;
> +  return {};
> +}
> +
>  obstack *gdbarch_obstack (gdbarch *arch)
>  {
>    return &arch->obstack;
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 1509cb7441e..14a84b74733 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -414,4 +414,9 @@ extern enum return_value_convention default_gdbarch_return_value
>        struct regcache *regcache, struct value **read_value,
>        const gdb_byte *writebuf);
>  
> +/* Default implementation of gdbarch default_get_shadow_stack_pointer
> +   method.  */
> +extern std::optional<CORE_ADDR> default_get_shadow_stack_pointer
> +  (gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled);
> +
>  #endif /* GDB_ARCH_UTILS_H */
> diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c
> index a4b72793fd8..caeda3cefae 100644
> --- a/gdb/gdbarch-gen.c
> +++ b/gdb/gdbarch-gen.c
> @@ -263,6 +263,7 @@ struct gdbarch
>    gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes = default_use_target_description_from_corefile_notes;
>    gdbarch_core_parse_exec_context_ftype *core_parse_exec_context = default_core_parse_exec_context;
>    gdbarch_shadow_stack_push_ftype *shadow_stack_push = nullptr;
> +  gdbarch_get_shadow_stack_pointer_ftype *get_shadow_stack_pointer = default_get_shadow_stack_pointer;
>  };
>  
>  /* Create a new ``struct gdbarch'' based on information provided by
> @@ -537,6 +538,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>    /* Skip verify of use_target_description_from_corefile_notes, invalid_p == 0.  */
>    /* Skip verify of core_parse_exec_context, invalid_p == 0.  */
>    /* Skip verify of shadow_stack_push, has predicate.  */
> +  /* Skip verify of get_shadow_stack_pointer, invalid_p == 0.  */
>    if (!log.empty ())
>      internal_error (_("verify_gdbarch: the following are invalid ...%s"),
>  		    log.c_str ());
> @@ -1414,6 +1416,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>    gdb_printf (file,
>  	      "gdbarch_dump: shadow_stack_push = <%s>\n",
>  	      host_address_to_string (gdbarch->shadow_stack_push));
> +  gdb_printf (file,
> +	      "gdbarch_dump: get_shadow_stack_pointer = <%s>\n",
> +	      host_address_to_string (gdbarch->get_shadow_stack_pointer));
>    if (gdbarch->dump_tdep != NULL)
>      gdbarch->dump_tdep (gdbarch, file);
>  }
> @@ -5583,3 +5588,20 @@ set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch,
>  {
>    gdbarch->shadow_stack_push = shadow_stack_push;
>  }
> +
> +std::optional<CORE_ADDR>
> +gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->get_shadow_stack_pointer != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_get_shadow_stack_pointer called\n");
> +  return gdbarch->get_shadow_stack_pointer (gdbarch, regcache, shadow_stack_enabled);
> +}
> +
> +void
> +set_gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch,
> +				      gdbarch_get_shadow_stack_pointer_ftype get_shadow_stack_pointer)
> +{
> +  gdbarch->get_shadow_stack_pointer = get_shadow_stack_pointer;
> +}
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 71142332540..c36171b089e 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1807,7 +1807,8 @@ extern void set_gdbarch_core_parse_exec_context (struct gdbarch *gdbarch, gdbarc
>     technologies.  For example, the Intel Control-Flow Enforcement Technology
>     (Intel CET) on x86 provides a shadow stack and indirect branch tracking.
>     To enable shadow stack support for inferior calls the shadow_stack_push
> -   gdbarch hook has to be provided.
> +   gdbarch hook has to be provided.  The get_shadow_stack_pointer gdbarch
> +   hook has to be provided to enable displaced stepping.
>  
>     Push NEW_ADDR to the shadow stack and update the shadow stack pointer. */
>  
> @@ -1816,3 +1817,12 @@ extern bool gdbarch_shadow_stack_push_p (struct gdbarch *gdbarch);
>  typedef void (gdbarch_shadow_stack_push_ftype) (struct gdbarch *gdbarch, CORE_ADDR new_addr, regcache *regcache);
>  extern void gdbarch_shadow_stack_push (struct gdbarch *gdbarch, CORE_ADDR new_addr, regcache *regcache);
>  extern void set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch, gdbarch_shadow_stack_push_ftype *shadow_stack_push);
> +
> +/* If possible, return the shadow stack pointer.  On some architectures, the
> +   shadow stack pointer is available even if the feature is disabled.  To
> +   return the feature's enablement state configure SHADOW_STACK_ENABLED.
> +   Set it to true in case the shadow stack is enabled. */
> +
> +typedef std::optional<CORE_ADDR> (gdbarch_get_shadow_stack_pointer_ftype) (struct gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled);
> +extern std::optional<CORE_ADDR> gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled);
> +extern void set_gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, gdbarch_get_shadow_stack_pointer_ftype *get_shadow_stack_pointer);
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index abc79588473..73459064170 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -2855,7 +2855,8 @@ Some targets support special hardware-assisted control-flow protection
>  technologies.  For example, the Intel Control-Flow Enforcement Technology
>  (Intel CET) on x86 provides a shadow stack and indirect branch tracking.
>  To enable shadow stack support for inferior calls the shadow_stack_push
> -gdbarch hook has to be provided.
> +gdbarch hook has to be provided.  The get_shadow_stack_pointer gdbarch
> +hook has to be provided to enable displaced stepping.

I find the addition of this last sentence here a little strange.  While
it's a true statement, wouldn't this be better placed on the comment for
get_shadow_stack_pointer?

>  
>  Push NEW_ADDR to the shadow stack and update the shadow stack pointer.
>  """,
> @@ -2864,3 +2865,17 @@ Push NEW_ADDR to the shadow stack and update the shadow stack pointer.
>      params=[("CORE_ADDR", "new_addr"), ("regcache *", "regcache")],
>      predicate=True,
>  )
> +
> +Method(
> +    comment="""
> +If possible, return the shadow stack pointer.  On some architectures, the
> +shadow stack pointer is available even if the feature is disabled.  To
> +return the feature's enablement state configure SHADOW_STACK_ENABLED.
> +Set it to true in case the shadow stack is enabled.

The wording "configure SHADOW_STACK_ENABLED" seems a little strange.
Also, there's a bunch of important detail that this comment doesn't
cover.  Here's what I'd suggest, though it's possible this doesn't match
the implementation (I haven't checked the next patch yet), but this does
match default_get_shadow_stack_pointer.  Feel free to take any of this
that is useful:

  If possible, return the shadow stack pointer.  On some architectures,
  the shadow stack pointer is available even if the feature is disabled.
  If the shadow stack feature is enabled then set SHADOW_STACK_ENABLED
  to true, otherwise set SHADOW_STACK_ENABLED to false.  The
  SHADOW_STACK_ENABLED will always be set if this function returns a
  value.  If the function doesn't return a value then the state of
  SHADOW_STACK_ENABLED is undefined.

Thanks,
Andrew


> +""",
> +    type="std::optional<CORE_ADDR>",
> +    name="get_shadow_stack_pointer",
> +    params=[("regcache *", "regcache"), ("bool &", "shadow_stack_enabled")],
> +    predefault="default_get_shadow_stack_pointer",
> +    invalid=False,
> +)
> -- 
> 2.43.0