RE: [PATCH v5 00/12] Add CET shadow stack support

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

Hi Luis,

I finally also tested x32 and it looks good, there was only one issue
with the new coredump test due to RDSSPD/RDSSPQ instructions
for reading the shadow stack pointer:
amd64-shadow-stack-corefile.c:26: Error: operand size mismatch for `rdsspq'

So I have to adapt the instruction for x32 to "rdsspd " to copy low 32 bits only.

I suggest to fix the testfile as follows:
~~~
diff --git a/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c b/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c
index f078e33810d..5e84793ccb1 100644
--- a/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c
+++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.c
@@ -23,7 +23,11 @@ void
 function ()
 {
   unsigned long ssp;
-  asm volatile("xor %0, %0; rdsspq %0" : "=r" (ssp));
+  #ifndef __ILP32__
+    asm volatile ("xor %0, %0; rdsspq %0" : "=r" (ssp));
+  #else
+    asm volatile ("xor %0, %0; rdsspd %0" : "=r" (ssp));
+  #endif
~~~

Would that be fine ore should I repost the patch?

Other than that I'll retest again with latest upstream master and push this tomorrow.

Thanks,
Christina

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Thursday, July 17, 2025 5:00 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; Andrew Burgess <aburgess@redhat.com>
> Subject: Re: [PATCH v5 00/12] Add CET shadow stack support
> 
> Hi,
> 
> On 7/17/25 13:01, Schimpe, Christina wrote:
> > Hi Luis,
> >
> > Thank you for the feedback.
> >
> >> -----Original Message-----
> >> From: Luis Machado <luis.machado@arm.com>
> >> Sent: Monday, July 14, 2025 9:13 AM
> >> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> >> patches@sourceware.org
> >> Cc: thiago.bauermann@linaro.org
> >> Subject: Re: [PATCH v5 00/12] Add CET shadow stack support
> >>
> >> On 7/13/25 20:05, Luis Machado wrote:
> >>> Hi,
> >>>
> >>> On 7/13/25 15:01, Schimpe, Christina wrote:
> >>>> Hi Luis,
> >>>>
> >>>> Thanks for sharing that info.
> >>>> I tried to reproduces with the same ubuntu & kernel versions and a
> >>>> very
> >> similar system but am not successful so far...
> >>>> Just to be sure, I assume you test with gcc compiler (which
> >>>> version?) and
> >> compiled GDB with latest upstream master + my cet shadow stack
> patches?
> >>>
> >>> Interesting. GCC says: gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0
> >>>
> >>> I applied the CET shadow stack patches from v5. And yes, gdb is the
> >>> latest
> >> tip-of-tree.
> >>>>
> >>>> Do you see this for target_board=native-gdbserver and
> >> target_board=unix?
> >>>
> >>> No. native-gdbserver, native-extended-gdbserver and unix work just fine.
> >>>
> >>> Let me have another look to see what might be going on.
> >>
> >> Heh, found it. I had a freshly-installed system and was missing
> >> libexpat-dev, so no XML was being generated.
> >>
> >> With libexpat-dev installed, everything looks fine. So this is good.
> >> Sorry for the noise.
> >
> > No worries!
> >
> >> I spotted two things while building gdb with the patch series applied
> >>
> >> There is a uninitialized usage warning
> >>
> >> gdb/nat/x86-linux.c:151:12: warning: 'ecx' may be used uninitialized
> >> [- Wmaybe-uninitialized]
> >>   151 |   if ((ecx & bit_SHSTK) == 0)
> >>       |       ~~~~~^~~~~~~~~~~~
> >> ../../../repos/binutils-gdb/gdb/nat/x86-linux.c:147:26: note: 'ecx'
> >> was declared here
> >>   147 |   unsigned int eax, ebx, ecx, edx;
> >
> > Yes, thank you for catching this. I'll fix it as follows:
> >
> > diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c index
> > 1756d5441fc..d037992d849 100644
> > --- a/gdb/nat/x86-linux.c
> > +++ b/gdb/nat/x86-linux.c
> > @@ -145,6 +145,7 @@ x86_check_ssp_support (const int tid)
> >       stack is not enabled for the current thread, we still want to return
> >       true.  */
> >    unsigned int eax, ebx, ecx, edx;
> > +  eax = ebx = ecx = edx = 0;
> >
> >> Also, git am seems to complain about some whitespace issues:
> >> Applying: gdb, gdbserver: Add support of Intel shadow stack pointer
> register.
> >> .git/rebase-apply/patch:596: indent with spaces.
> >>             # Having unavailable registers leads to a fall back to
> >> the standard
> >> .git/rebase-apply/patch:597: indent with spaces.
> >>             # unwinders.  Don't add unavailable registers to avoid this.
> >> .git/rebase-apply/patch:598: indent with spaces.
> >>             if (str (val) == "<unavailable>"):
> >> .git/rebase-apply/patch:599: indent with spaces.
> >>                 continue
> >
> > I would assume that these findings are false positives, since it's a
> > .py file and the idents are consistently with spaces in that file.
> > Is that ok?
> 
> It might be. If a false positive, feel free to ignore it.
> 
> >
> >> .git/rebase-apply/patch:587: new blank line at EOF.
> >> +
> >
> > Thanks! Will fix.
> >
> > I also spotted a few whitespace issues through the series:
> >
> > diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index
> > 782b66f1467..a2b49730e00 100644
> > --- a/gdb/amd64-linux-tdep.c
> > +++ b/gdb/amd64-linux-tdep.c
> > @@ -1981,7 +1981,7 @@ amd64_linux_shadow_stack_push (gdbarch
> *gdbarch, CORE_ADDR new_addr,
> >      return;
> >
> >    /* The shadow stack grows downwards.  To push addresses to the stack,
> > -     we need to decrement SSP.    */
> > +     we need to decrement SSP.  */
> >    const int element_size
> >      = amd64_linux_shadow_stack_element_size_aligned (gdbarch);
> >    const CORE_ADDR new_ssp = *ssp - element_size; @@ -2004,7 +2004,7
> > @@ amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR
> new_addr,
> >    regcache_raw_write_unsigned (regcache, tdep->ssp_regnum, new_ssp);
> > }
> >
> > -/* Implement shadow stack pointer unwinding. For each new shadow
> > stack
> > +/* Implement shadow stack pointer unwinding.  For each new shadow
> > +stack
> >     pointer check if its address is still in the shadow stack memory range.
> >     If it's outside the range set the returned value to unavailable,
> >     otherwise return a value containing the new shadow stack pointer.
> > */ diff --git a/gdb/arch/x86-linux-tdesc-features.h
> > b/gdb/arch/x86-linux-tdesc-features.h
> > index d1d74e70df8..a280bf608d7 100644
> > --- a/gdb/arch/x86-linux-tdesc-features.h
> > +++ b/gdb/arch/x86-linux-tdesc-features.h
> > @@ -58,7 +58,7 @@ extern int x86_linux_i386_tdesc_count ();
> >     x86_linux_xstate_bv_mask_to_tdesc_idx) into an xstate_bv_mask
> >     value which can then be used to create a target description.
> >     The return mask has the same format than the state component bitmap
> > -   and does include user and supervisor state components.*/
> > +   and does include user and supervisor state components.  */
> >
> >  extern uint64_t x86_linux_tdesc_idx_to_xstate_bv_mask (int idx);
> >
> > diff --git a/gdb/testsuite/gdb.arch/amd64-ssp.exp
> > b/gdb/testsuite/gdb.arch/amd64-ssp.exp
> > index 6ddc875b9a3..18d43065034 100644
> > --- a/gdb/testsuite/gdb.arch/amd64-ssp.exp
> > +++ b/gdb/testsuite/gdb.arch/amd64-ssp.exp
> > @@ -47,4 +47,3 @@ save_vars { ::env(GLIBC_TUNABLES) } {
> >      # configuring the shadow stack pointer.
> >      gdb_continue_to_end
> >  }
> > -
> > diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
> > index 1a0393d127c..beb04b7f8f6 100644
> > --- a/gdbserver/linux-i386-ipa.cc
> > +++ b/gdbserver/linux-i386-ipa.cc
> > @@ -174,7 +174,7 @@ initialize_fast_tracepoint_trampoline_buffer
> > (void)  const struct target_desc *  get_ipa_tdesc (int idx)  {
> > -  uint64_t xstate_bv_mask = x86_linux_tdesc_idx_to_xstate_bv_mask
> > (idx);
> > +  uint64_t xstate_bv_mask = x86_linux_tdesc_idx_to_xstate_bv_mask
> > + (idx);
> >
> >> warning: 5 lines add whitespace errors.
> >
> > which I plan to fix locally, too.
> >
> >>
> >> Could you please address those locally?
> >>
> >> Otherwise...
> >>
> >> Approved-By: Luis Machado <luis.machado@arm.com>
> >>
> >> I'd give this another week in case someone has any more comments, and
> >> then push it.
> >>
> >> Thanks for the series.
> >
> > My plan is to address your feedback as described here and also Andrew's
> feedback for the v5.
> > So far I haven't received any feedback that would block merging this
> > or requires a new version think, but my plan would be to wait until
> > middle next week. I also plan to include parts of Thiago's patch "gdb,
> > testsuite: Extend core_find procedure to save program output" (first patch
> of v5), if his series will not be merged before mine.
> >
> > Please let me know if you have any concerns with that plan.
> That sounds reasonable to me.
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