Re: [PATCH v4 09/11] gdb: Implement amd64 linux shadow stack support for inferior calls.

Luis Machado
2025-06-28T10:39:21Z

On 6/27/25 20:52, Schimpe, Christina wrote:
> Hi Luis,
>
> Thanks for the review.
>
> <...>
>
>>> +
>>> + /* Starting with v6.6., the Linux kernel supports CET shadow stack.
>>
>> Typo, period after "v6.6".
>>
>>> + Dependent on the target the ssp register can be invalid or nullptr
>>> + when shadow stack is supported by HW and the linux kernel but not
>>> + enabled for the current thread. */
>>
>> I feel the comment doesn't quite reflect this case very well. We don't have a
>> nullptr here, so I suppose ssp == 0x0 means ssp is unavailable? We should
>> make that clear here, but we don't need to have this more generic comment
>> pasted here again, as it was explained elsewhere already.
>
> I agree. I removed the "Starting with v6.6., the Linux kernel supports..." comment
> and rewrote the sentence afterwards as follows:
>
> "Dependent on the target in case the shadow stack pointer is unavailable, the ssp
> register can be invalid or 0x0."
>
> I hope this is a bit better.
>

That sounds better to me. Thanks.

>>> + if (ssp == 0x0)
>>> + return {};
>>> +
>>> + return ssp;
>>> +}
>>> +
>>> +/* If shadow stack is enabled, push the address NEW_ADDR on the
>>> +shadow
>>
>> s/the address NEW_ADDR on/NEW_ADDR to
>>
>>> + stack and increment the shadow stack pointer accordingly. */
>>> +
>>> +static void
>>> +amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR
>> new_addr,
>>> + regcache *regcache)
>>> +{
>>> + std::optional<CORE_ADDR> ssp
>>> + = amd64_linux_get_shadow_stack_pointer (gdbarch, regcache);
>>> + if (!ssp.has_value ())
>>> + return;
>>> +
>>> + /* The shadow stack grows downwards. To push addresses on the
>>> + stack,
>>
>> s/on the/to the
>>
>>> + 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;
>>> +
>>> + /* Starting with v6.6., the Linux kernel supports CET shadow stack.
>>
>> Same typo, period after "v6.6". I feel this comment has been repeated
>> enough times throughout the code. If it is available at a visible location, I
>> think we can do without it elsewhere.
>
> I agree.
>
>> For instance, we could even mention the
>> kernel version in the news entry, or at the gdbarch initialization code when
>> we are fetching a target description. Then it should be enough.
>
> I would suggest to remove the comment
>
> "Starting with v6.6., the Linux kernel supports CET shadow stack."
>
> everywhere except in the allow_ssp_tests procedure, as it fits best there in my opinion.
>
> Would that be acceptable?

I think so. If you want to mention it somewhere in the gdb sources, then maybe the gdbarch initialization
function would be another acceptable spot.

>
> <...>
>
>> Otherwise this is OK.
>>
>> Reviewed-By: Luis Machado <luis.machado@arm.com>
>
> I actually missed to reply to this feedback in the first place. Sorry for the late reply.
> I'll post my v5 soon, as I'll be out for one week. If anything suggested above is not acceptable,
> it would be great if you could let me know.
> Since this is affecting comments only, I think it should be straight-forward to fix.
> For now I'll post my v5 as suggested above.

No worries. I see v5 on the list and will go through it. Have a nice break.