Re: [PATCH 06/12] gdb, gdbserver: Add support of Intel shadow stack pointer register.

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

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

> This patch adds the user mode register PL3_SSP which is part of the
> Intel(R) Control-Flow Enforcement Technology (CET) feature for support
> of shadow stack.
> For now, only native and remote debugging support for shadow stack
> userspace on amd64 linux are covered by this patch including 64 bit and
> x32 support.  32 bit support is not covered due to missing linux kernel

Nit: Linux (uppercase l)

> support.

<snip>

> diff --git a/gdb/arch/i386.c b/gdb/arch/i386.c
> index 4a39028a472..59daaa4c583 100644
> --- a/gdb/arch/i386.c
> +++ b/gdb/arch/i386.c
> @@ -28,6 +28,7 @@
>  #include "../features/i386/32bit-avx512.c"
>  #include "../features/i386/32bit-segments.c"
>  #include "../features/i386/pkeys.c"
> +#include "../features/i386/32bit-ssp.c"
>
>  /* See i386.h.  */
>
> @@ -66,5 +67,8 @@ i386_create_target_description (uint64_t xstate_bv_mask, bool is_linux,
>    if (xstate_bv_mask & X86_XSTATE_PKRU)
>      regnum = create_feature_i386_pkeys (tdesc.get (), regnum);
>
> +  if (xstate_bv_mask & X86_XSTATE_CET_U)
> +    regnum = create_feature_i386_32bit_ssp (tdesc.get (), regnum);
> +
>    return tdesc.release ();
>  }

The patch description mentions that "32 bit support is not covered due
to missing linux kernel support". Is this change useful, or is it
unreachable code?

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index a86f534528c..fc35456f1d3 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4225,6 +4225,75 @@ gdb_caching_proc allow_tsx_tests {} {
>      return $allow_tsx_tests
>  }
>
> +# Run a test on the target to check if it supports x86 shadow stack.  Return 1
> +# if shadow stack is enabled, 0 otherwise.
> +
> +gdb_caching_proc allow_ssp_tests {} {
> +    global srcdir subdir gdb_prompt hex
> +
> +    set me "allow_ssp_tests"
> +
> +    if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +	verbose "$me: target known to not support shadow stack."
> +	return 0
> +    }
> +
> +    # There is no need to check the actual HW in addition to ptrace support.
> +    # We need both checks and ptrace will tell us about the HW state.
> +    set compile_flags "{additional_flags=-fcf-protection=return}"
> +    set src { int main() { return 0; } }
> +    if {![gdb_simple_compile $me $src executable $compile_flags]} {
> +	return 0
> +    }
> +
> +    save_vars { ::env(GLIBC_TUNABLES) } {
> +
> +	append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
> +
> +	# No error message, compilation succeeded so now run it via gdb.
> +	gdb_exit
> +	gdb_start
> +	gdb_reinitialize_dir $srcdir/$subdir
> +	gdb_load $obj
> +	if {![runto_main]} {
> +	    return 0

There should be a call to "remote_file build delete $obj" here as well.

> +	}
> +	set shadow_stack_disabled_re "(<unavailable>)"
> +	if {[istarget *-*-linux*]} {
> +	    # Starting with v6.6., the Linux kernel supports CET shadow stack.
> +	    # Dependent on the target we can see a nullptr or "<unavailable>"
> +	    # when shadow stack is supported by HW and the linux kernel but

Nit: Linux

> +	    # not enabled for the current thread (for example due to a lack
> +	    # of compiler or glibc support for -fcf-protection).
> +	    set shadow_stack_disabled_re "$shadow_stack_disabled_re|(.*0x0)"
> +	}

<snip>

> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
> index d1fece717a7..5bbd4640e30 100644
> --- a/gdb/x86-linux-nat.c
> +++ b/gdb/x86-linux-nat.c
> @@ -41,6 +41,7 @@
>  #include "nat/x86-linux.h"
>  #include "nat/x86-linux-dregs.h"
>  #include "nat/linux-ptrace.h"
> +#include "x86-tdep.h"
>  #include "nat/x86-linux-tdesc.h"
>
>  /* linux_nat_target::low_new_fork implementation.  */
> @@ -97,11 +98,10 @@ const struct target_desc *
>  x86_linux_nat_target::read_description ()
>  {
>    /* The x86_linux_tdesc_for_tid call only reads xcr0 the first time it is
> -     called.  The mask is stored in XSTATE_BV_STORAGE and reused on
> -     subsequent calls.  Note that GDB currently supports features for user
> -     state components only.  However, once supervisor state components are
> -     supported in GDB XSTATE_BV_STORAGE will not be configured based on
> -     xcr0 only.  */
> +     called.  Also it checks the enablement state of features which are
> +     not configured in xcr0, such as CET shadow stack.  Once the supported

The "not" above should be removed.

> +     features are identified, the XSTATE_BV_STORAGE value is configured
> +     accordingly and preserved for subsequent calls of this function.  */
>    static uint64_t xstate_bv_storage;
>
>    if (inferior_ptid == null_ptid)
> @@ -215,6 +215,46 @@ x86_linux_get_thread_area (pid_t pid, void *addr, unsigned int *base_addr)
>  }
>  \f
>
> +/* See x86-linux-nat.h.  */
> +
> +void
> +x86_linux_fetch_ssp (regcache *regcache, const int tid)
> +{
> +  uint64_t ssp = 0x0;
> +  iovec iov {&ssp, sizeof (ssp)};
> +
> +  /* The shadow stack may be enabled and disabled at runtime.  Reading the
> +     ssp might fail as shadow stack was not activated for the current
> +     thread.  We don't want to show a warning but silently return.  The
> +     register will be shown as unavailable for the user.  */
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_X86_SHSTK, &iov) != 0)
> +    return;

In case the ptrace fails and there is already an old value for ssp in
regcache, shouldn't it be removed from it?

> +
> +  x86_supply_ssp (regcache, ssp);
> +}
> +
> +/* See x86-linux-nat.h.  */
> +
> +void
> +x86_linux_store_ssp (const regcache *regcache, const int tid)
> +{
> +  uint64_t ssp = 0x0;
> +  iovec iov {&ssp, sizeof (ssp)};
> +  x86_collect_ssp (regcache, ssp);
> +
> +  /* Starting with v6.6., the Linux kernel supports CET shadow stack.
> +     Dependent on the target the ssp register can be unavailable or
> +     nullptr when shadow stack is supported by HW and the linux kernel but

Nit: Linux

> +     not enabled for the current thread.  In case of nullptr, GDB tries to
> +     restore the shadow stack pointer after an inferior call.  The ptrace
> +     call with PTRACE_SETREGSET will fail here with errno ENODEV.  We
> +     don't want to throw an error in this case but silently continue.  */
> +  errno = 0;
> +  if ((ptrace (PTRACE_SETREGSET, tid, NT_X86_SHSTK, &iov) != 0)
> +      && (errno != ENODEV))
> +    perror_with_name (_("Failed to write pl3_ssp register"));

Same question here: if the ptrace call fails shouldn't the ssp value in
regcache be removed?

> +}
> +
>  void _initialize_x86_linux_nat ();
>  void
>  _initialize_x86_linux_nat ()
> diff --git a/gdb/x86-linux-nat.h b/gdb/x86-linux-nat.h
> index 3c2241bb0b6..d5dc1908090 100644
> --- a/gdb/x86-linux-nat.h
> +++ b/gdb/x86-linux-nat.h
> @@ -92,4 +92,15 @@ struct x86_linux_nat_target : public x86_nat_target<linux_nat_target>
>  extern ps_err_e x86_linux_get_thread_area (pid_t pid, void *addr,
>  					   unsigned int *base_addr);
>
> +/* Fetch the value of the shadow stack pointer register from process/thread
> +   TID and store it to GDB's register cache.  */
> +
> +extern void x86_linux_fetch_ssp (regcache *regcache, const int tid);
> +
> +/* Read the value of the shadow stack pointer from GDB's register cache
> +   and store it in the shadow stack pointer register of process/thread TID.
> +   Throw an error in case of failure.  */
> +
> +extern void x86_linux_store_ssp (const regcache *regcache, const int tid);
> +
>  #endif /* GDB_X86_LINUX_NAT_H */
> diff --git a/gdb/x86-tdep.c b/gdb/x86-tdep.c
> index e50b5fb9fa4..08fb0e8d82d 100644
> --- a/gdb/x86-tdep.c
> +++ b/gdb/x86-tdep.c
> @@ -17,10 +17,32 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>
> +#include "defs.h"

defs.h is now included in all GDB files via the gcc command line, and
shouldn't be #included anymore. See commit 18d2988e5da8 ("gdb,
gdbserver, gdbsupport: remove includes of early headers") and commit
ab7daea3ad0d ("gdb, gdbserver, gdbsupport: include early header files
with `-include`").

> +#include "i386-tdep.h"
>  #include "x86-tdep.h"
>  #include "symtab.h"

--
Thiago