Re: [PATCH] GDB: aarch64: Fix inferior function call if GCS is present but disabled

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Baris,

Thank you for your review!

"Aktemur, Baris" <TankutBaris.Aktemur@amd.com> writes:

>> On AArch64, even if the Guarded Control Stack feature is present on the
>> system the inferior may not have enabled it.
>>
>> There's a bug in GDB's GCS support in that if the system supports GCS,
>> GDB will always push a GCS entry when doing an inferior function call
>> even if the inferior doesn't have it enabled.  This causes inferior
>> function calls to fail. E.g.:
>>
>>   (gdb) p foo ()
>>   Can't write to Guarded Control Stack.
>>
>> The error message is because the GCSPR doesn't point to a valid memory
>> address.
>>
>> Fix by checking whether GCS is enabled in the inferior before trying to
>> push a GCS entry.
>>
>> Regression tested on an aarch64-linux-gnu QEMU VM with GCS present.
>> ---
>>
>> GCS support first appeared in GDB 17, so if this patch is approved for
>> trunk is it ok to also commit it to the branch?
>>
>>  gdb/aarch64-tdep.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 886e7ce1e7b6..b4437dcccc8a 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -1934,7 +1934,11 @@ static void
>>  aarch64_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR new_addr,
>>                            regcache *regcache)
>>  {
>> -  aarch64_push_gcs_entry (regcache, new_addr);
>> +  bool gcs_is_enabled;
>> +
>> +  gdbarch_get_shadow_stack_pointer (gdbarch, regcache, gcs_is_enabled);
>
> Some analyzer tools may flag this because the return value is not being used.
> Maybe put a `(void)` in front to indicate the return value is being ignored
> deliberately.

Makes sense. I made this change locally:

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b4437dcccc8a..5b38ff74260c 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1936,7 +1936,7 @@ aarch64_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR new_addr,
 {
   bool gcs_is_enabled;
 
-  gdbarch_get_shadow_stack_pointer (gdbarch, regcache, gcs_is_enabled);
+  (void) gdbarch_get_shadow_stack_pointer (gdbarch, regcache, gcs_is_enabled);
   if (gcs_is_enabled)
     aarch64_push_gcs_entry (regcache, new_addr);
 }

-- 
Thiago
(he/him)