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

Luis Machado
2025-07-17T15:00:53Z

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.