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

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

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.