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

Matthieu Longo matthieu.longo@arm.com
Wed Dec 11 11:11:35 GMT 2024 

On 2024-12-10 18:00, Richard Earnshaw (lists) wrote:
> 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. 

The new test cfi-sframe-aarch64-3.s is introduced to ensure that GCC-14 
or earlier continue to accept .cfi_window_save as it used to be. This is 
the backward compatibility test.

.cfi_window_save can be tested separately from the A and B keys. The 
.cfi_b_key_frame directive was not changed, so it is working as it 
always has worked.

The test cfi-sframe-aarch64-pac-ab-key-1.s was updated to use the new 
directive. I chose to replace it because we are supposed to use 
.cfi_negate_ra_state instead. If someone looks at the tests, he will see
.cfi_negate_ra_state which is used everywhere except in a backward 
compatibility test, so it is clearly *the recommended way*.

In the same way, in the previous commit, pac_ab_key.s was updated to use 
.cfi_negate_ra_state and a new test pac_compat_cfi_window_save.s was 
added to ensure the backward compatibility.

I hope the situation is clearer now. I probably should have explained it 
better in the commit message.

Matthieu

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

More information about the Binutils mailing list