[PATCH v1 2/4] aarch64 SFrame: use preferred CFI directive for AArch64 PAC

[Matthieu Longo]

On 2024-12-01 03:23, Indu Bhagat wrote:
> On 11/25/24 8:28 AM, Matthieu Longo wrote:
>> ARMv8.3 addded support for a new security feature named Pointer
>> Authentication. Support for this feature in SFrame already exists,
>> but is relying on the deprecated "AArch64" (actually Sparc) CFI
>> directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
>> should be used instead to convey this information.
>>
>> In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
>> is emitted instead of .cfi_negate_ra_state.
>> GCC 15 fixed this issue, but this behavior is preserved for backward
>> compatibility.
>>
>> The existing sframe test for AArch64 PAC was using .cfi_gnu_window_save.
>> This patch replaces this CFI in the existing test by the preferred one,
>> and adds a new test to check for backward compatibility when using
>> .cfi_gnu_window_save.
> 
> Hi Matthieu,
> 
> See one comment inlined below.
> 
> Thanks
> Indu
> 
>> ---
>>   .../gas/cfi-sframe/cfi-sframe-aarch64-3.d     | 20 ++++++++++++++++++
>>   .../gas/cfi-sframe/cfi-sframe-aarch64-3.s     | 21 +++++++++++++++++++
>>   .../cfi-sframe-aarch64-pac-ab-key-1.s         |  8 +++----
>>   gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |  1 +
>>   4 files changed, 46 insertions(+), 4 deletions(-)
>>   create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>>   create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d b/ 
>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>> new file mode 100644
>> index 00000000000..f72b70a970a
>> --- /dev/null
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>> @@ -0,0 +1,20 @@
>> +#as: --gsframe
>> +#objdump: --sframe=.sframe
>> +#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
>> +#...
>> +Contents of the SFrame section .sframe:
>> +
>> +  Header :
>> +
>> +    Version: SFRAME_VERSION_2
>> +    Flags: NONE
>> +    Num FDEs: 1
>> +    Num FREs: 2
>> +
>> +  Function Index :
>> +    func idx \[0\]: pc = 0x0, size = 8 bytes
>> +    STARTPC + CFA + FP + RA +
>> +#...
>> +    0+0004 +sp\+16 +u +u\[s\] +
>> +
>> +#pass
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s b/ 
>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>> new file mode 100644
>> index 00000000000..df6ff6e8c8f
>> --- /dev/null
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>> @@ -0,0 +1,21 @@
>> +## ARMv8.3 addded support a new security feature named Pointer 
>> Authentication. The
>> +## main idea behind this is to use the unused bits in the pointer 
>> values.
>> +## Each pointer is patched with a PAC before writing to memory, and 
>> is verified
>> +## before using it.
>> +## When the pointers are mangled, the stack trace generator needs to 
>> know so it
>> +## can mask off the PAC from the pointer value to recover the return 
>> address,
>> +## and conversely, skip doing so if the pointers are not mangled.
>> +##
>> +## .cfi_negate_ra_state CFI directive is usually used to convey this 
>> information.
>> +## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save is 
>> emitted
>> +## instead of .cfi_negate_ra_state.  GCC 15 fixed this issue, but 
>> this behavior
>> +## is preserved in binutils for backward compatibility.
>> +##
>> +## SFrame has support for this. This testcase ensures that the directive
>> +## is interpreted successfully.
>> +    .cfi_startproc
>> +    .long 0
>> +    .cfi_def_cfa_offset 16
>> +    .cfi_window_save
>> +    .long 0
>> +    .cfi_endproc
> 
> Since the purpose is to ensure that the old behavior is maintained, I am 
> wondering if it is not better to keep the old test as is (which also 
> tests the .cfi_window_save *together with* the .cfi_b_key_frame 
> directives). WDYT ?
> 

The old behavior is not going to be maintained with GCC 15 and newer.
 From my perspective, the purpose of this test is to test the behavior 
of A and B keys with the *correct* AArch64 CFI directives. That's the 
reason why I added a separate test cfi-sframe-aarch64-3.s to ensure that 
the old directive is still correctly parsed and processed.

Matthieu.

>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab- 
>> key-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>> index d9a408c668c..84230a99e3c 100644
>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>> @@ -8,12 +8,12 @@ _Z5foo_av:
>>   .LFB0:
>>       .cfi_startproc
>>       hint    25 // paciasp
>> -    .cfi_window_save
>> +    .cfi_negate_ra_state
>>       stp    x29, x30, [sp, -16]!
>>       .cfi_def_cfa_offset 16
>>       .cfi_offset 29, -16
>>       .cfi_offset 30, -8
>> -        ret
>> +    ret
>>       .cfi_endproc
>>   .LFE0:
>>       .size    _Z5foo_av, .-_Z5foo_av
>> @@ -25,12 +25,12 @@ _Z5foo_bv:
>>       .cfi_startproc
>>       .cfi_b_key_frame
>>       hint    27 // pacibsp
>> -    .cfi_window_save
>> +    .cfi_negate_ra_state
>>       stp    x29, x30, [sp, -16]!
>>       .cfi_def_cfa_offset 16
>>       .cfi_offset 29, -16
>>       .cfi_offset 30, -8
>>       nop
>>       nop
>> -        ret
>> +    ret
>>       .cfi_endproc
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/ 
>> testsuite/gas/cfi-sframe/cfi-sframe.exp
>> index cfae28d39aa..c646b109895 100644
>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> @@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] && [gas_sframe_check] } 
>> then {
>>   if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
>>       run_dump_test "cfi-sframe-aarch64-1"
>>       run_dump_test "cfi-sframe-aarch64-2"
>> +    run_dump_test "cfi-sframe-aarch64-3"
>>       run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
>>   }
>