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

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Thiago,

Thank you for the review. Please see my comments to your feedback below.

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Thursday, February 6, 2025 4:14 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 06/12] gdb, gdbserver: Add support of Intel shadow stack
> pointer register.
> 
> 
> "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.

Will fix.
 
> <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?

I think I consistently did not implement 32 bit linux support, but added the code
for 32 bit support in locations which are not linux dependent, as preparation for
other OS. But for now yes, this should be unreachable code.
Should I (1) Remove the code or (2) improve the commit message ?
I'd be in favour of (2),  would that be acceptable?

> > 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.

Thanks for catching that. I'll fix it.

> > +	}
> > +	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

Will fix.

> > +	    # 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.

You mean the "not" in this sentence?
"Also it checks the enablement state of features which are not configured in xcr0, such as CET shadow stack.  "

This should be correct in that context. CET shadow stack is not configured in xcr0.

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

Hm, it doesn't seem to be a problem. I ran a test using inline enablement of shadow stack:
~~~
(gdb) p $pl3_ssp
$1 = (void *) 0x7ffff7c00000
(gdb) n
54	   if (ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK)) {
(gdb) n
58	   return ret;
(gdb) p $pl3_ssp
$2 = <unavailable>
~~~
So it should be fine from my perspective. Or do you see another potential issue?

> > +
> > +  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

Will fix.

> > +     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?

Please see my answer above.

> > +}
> > +
> >  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`").

Will fix, thanks for pointing that out.

Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928