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

[Luis Machado <luis.machado@arm.com>]

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.