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

[Richard Earnshaw (lists)]

On 10/12/2024 11:13, Matthieu Longo wrote:
> 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.

We do need to ensure that a new version of gas works with old versions 
of GCC, so if GCC-14 or earlier emit the wrong directive we will need to 
continue to accept it until such versions are truly obsolete (a long time).

R.

> 
> 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"
>>>   }
>>
>