[PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

Sam Tebbs
Hi all,

Armv8.3-A has another key used in pointer authentication called the B-key (other
than the A-key that is already supported). In order for stack unwinders to work
it is necessary to be able to identify frames that have been signed with the
B-key rather than the A-key and it was felt that keeping this as an augmentation
character in the CIE was the best bet. The DWARF extensions for ARM therefore
propose to add a new augmentation character 'B' to the CIE augmentation string
and the corresponding cfi directive ".cfi_b_key_frame". I've made the relevant
changes to GAS and LD to add support for B-key unwinding, which required
modifying LD to check for 'B' in the augmentation string, adding the
".cfi_b_key_frame" directive to GAS and adding a "pauth_key" field to GAS's
fde_entry and cie_entry structs.

The pointer authentication instructions will behave as NOPs on architectures
that don't support them, and so a check for the architecture being assembled
for is not necessary since there will be no behavioural difference between
augmentation strings with and without the 'B' character on such architectures.

Built on aarch64-linux-gnu and aarch64-none-elf, and tested on aarch64-none-elf
with no regressions. This patch has been tested with the corresponding patch
that enables B-key support in GCC.

OK for trunk? I don't have write access so I'd appreciate someone committing
after approval.

bfd/
2018-11-01  Sam Tebbs<[hidden email]>

        * elf-eh-frame.c (_bfd_elf_parse_eh_frame): Add check for 'B'.

gas/
2018-11-01  Sam Tebbs<[hidden email]>

        * dw2gencfi.c (struct cie_entry): Add pauth_key field.
        (alloc_fde_entry): Set pauth_key to AARCH64_PAUTH_KEY_A by default.
        (output_cie): Output 'B' if pauth_key == AARCH64_PAUTH_KEY_B.
        (select_cie_for_fde): Add check for pauth_key. Set cie's pauth_key to
        fde's pauth_key.
        (frch_cfi_data, cfa_save_data): Move to dwgencfi.h.
        * config/tc-aarch64.c (dot_cfi_b_key_frame): Declare.
        (md_pseudo_table): Add "cfi_b_key_frame".
        * dw2gencfi.h (pointer_auth_key): Define.
        (struct fde_entry): Add pauth_key field.
        (frch_cfi_data, cfa_save_data): Move from dwgencfi.c.

gas/doc/
2018-11-01  Sam Tebbs<[hidden email]>

        * c-aarch64.texi (.cfi_b_key_frame): Add documentation.

gas/testsuite
2018-11-01  Sam Tebbs<[hidden email]>

        * gas/aarch64/(pac_ab_key.d, pac_ab_key.s): New file.


latest.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

Sam Tebbs
On 11/01/2018 01:23 PM, Sam Tebbs wrote:

> Hi all,
>
> Armv8.3-A has another key used in pointer authentication called the
> B-key (other
> than the A-key that is already supported). In order for stack
> unwinders to work
> it is necessary to be able to identify frames that have been signed
> with the
> B-key rather than the A-key and it was felt that keeping this as an
> augmentation
> character in the CIE was the best bet. The DWARF extensions for ARM
> therefore
> propose to add a new augmentation character 'B' to the CIE
> augmentation string
> and the corresponding cfi directive ".cfi_b_key_frame". I've made the
> relevant
> changes to GAS and LD to add support for B-key unwinding, which required
> modifying LD to check for 'B' in the augmentation string, adding the
> ".cfi_b_key_frame" directive to GAS and adding a "pauth_key" field to
> GAS's
> fde_entry and cie_entry structs.
>
> The pointer authentication instructions will behave as NOPs on
> architectures
> that don't support them, and so a check for the architecture being
> assembled
> for is not necessary since there will be no behavioural difference
> between
> augmentation strings with and without the 'B' character on such
> architectures.
>
> Built on aarch64-linux-gnu and aarch64-none-elf, and tested on
> aarch64-none-elf
> with no regressions. This patch has been tested with the corresponding
> patch
> that enables B-key support in GCC.
>
> OK for trunk? I don't have write access so I'd appreciate someone
> committing
> after approval.
>
> bfd/
> 2018-11-01  Sam Tebbs<[hidden email]>
>
>     * elf-eh-frame.c (_bfd_elf_parse_eh_frame): Add check for 'B'.
>
> gas/
> 2018-11-01  Sam Tebbs<[hidden email]>
>
>     * dw2gencfi.c (struct cie_entry): Add pauth_key field.
>     (alloc_fde_entry): Set pauth_key to AARCH64_PAUTH_KEY_A by default.
>     (output_cie): Output 'B' if pauth_key == AARCH64_PAUTH_KEY_B.
>     (select_cie_for_fde): Add check for pauth_key. Set cie's pauth_key to
>     fde's pauth_key.
>     (frch_cfi_data, cfa_save_data): Move to dwgencfi.h.
>     * config/tc-aarch64.c (dot_cfi_b_key_frame): Declare.
>     (md_pseudo_table): Add "cfi_b_key_frame".
>     * dw2gencfi.h (pointer_auth_key): Define.
>     (struct fde_entry): Add pauth_key field.
>     (frch_cfi_data, cfa_save_data): Move from dwgencfi.c.
>
> gas/doc/
> 2018-11-01  Sam Tebbs<[hidden email]>
>
>     * c-aarch64.texi (.cfi_b_key_frame): Add documentation.
>
> gas/testsuite
> 2018-11-01  Sam Tebbs<[hidden email]>
>
>     * gas/aarch64/(pac_ab_key.d, pac_ab_key.s): New file.
>

ping

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

Nick Clifton
In reply to this post by Sam Tebbs
Hi Sam,

> The DWARF extensions for ARM therefore
> propose to add a new augmentation character 'B' to the CIE augmentation string
> and the corresponding cfi directive ".cfi_b_key_frame".

Are these extensions documented somewhere ?  And is the documentation referenced
in the source code so that readers can find the description ?


> The pointer authentication instructions will behave as NOPs on architectures
> that don't support them, and so a check for the architecture being assembled
> for is not necessary since there will be no behavioural difference between
> augmentation strings with and without the 'B' character on such architectures.

Except that if the new .cfi_b_key_frame pseudo-op is used on an architecture
that does not support it, the programmer might expect to see a warning message,
yes ?  (Although reading the patch, it looks like the new pseudo-op is only
defined for the AArch64 so other architectures cannot even use it).


> Built on aarch64-linux-gnu and aarch64-none-elf, and tested on aarch64-none-elf
> with no regressions.

Did you also test on an non AArch64 configuration, just to be sure,  eg x86_64-pc-linux-gnu ?

It would be nice if you also updated binutils/dwarf.c:read_cie() so that it explicitly
handles the 'A' and 'B' encodings, even if there is nothing for it to do.  Just so
that readers know that the encodings are recognised.  (And maybe unrecognised encodings
should be reported....)


Some comments on the patch itself:

> diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c

>   switch (*aug++)
>    {
> +  case 'B':
> +    break;

I would feel much happier if these encoding characters were defined once in
a header somewhere and then #included wherever they were needed.  It is not
necessary to make this change for this patch submission, but it would be a
nice thing to have, if not now, then in the future.

 > diff --git a/gas/dw2gencfi.h b/gas/dw2gencfi.h

> +enum pointer_auth_key {
> +  AARCH64_PAUTH_KEY_A,
> +  AARCH64_PAUTH_KEY_B
> +};

I do not like having target specific enums defined in a generic
header file.  I would prefer to see definitions like this in
gas/config/tc-aarch64.h.


> @@ -162,6 +183,8 @@ struct fde_entry
>    /* For out of line tables and FDEs.  */
>    symbolS *eh_loc;
>    int sections;
> +  /* The pointer authentication key used.  Only used for AArch64.  */
> +  enum pointer_auth_key pauth_key;
>  };

Similarly, I think that this extra field should target defined.
Ie something like:

    int sections;
  #ifdef tc_fde_entry_extras
    tc_fde_entry_extras
  #endif
  };

And then define tc_fde_entry_extras in tc-aarch64.h.  This would
also allow other backends to extend the fde_entry structure if they
even find a need to do something similar.

> diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
> @@ -403,6 +403,7 @@ struct cie_entry
>    unsigned char per_encoding;
>    unsigned char lsda_encoding;
>    expressionS personality;
> +  enum pointer_auth_key pauth_key;
>    struct cfi_insn_data *first, *last;
>  };

A similar comment applies here.

> @@ -448,6 +433,7 @@ alloc_fde_entry (void)
>    fde->per_encoding = DW_EH_PE_omit;
>    fde->lsda_encoding = DW_EH_PE_omit;
>    fde->eh_header_type = EH_COMPACT_UNKNOWN;
> +  fde->pauth_key = AARCH64_PAUTH_KEY_A;
>  
>    return fde;
>  }

And of course this would have to be:

     fde->eh_header_type = EH_COMPACT_UNKNOWN;
  #ifdef tc_fde_entry_init
     tc_fde_entry_init (fde)
  #endif

And so on for the other changes in dw2gencfi.c.  I realise that this
is a pain to do, but I would like to keep generic code generic, and
make it clear where target specific overrides are happening.


Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

Sam Tebbs
On 11/09/2018 01:27 PM, Nick Clifton wrote:

> Hi Sam,

Hi Nick, thanks for the feedback.

>> The DWARF extensions for ARM therefore
>> propose to add a new augmentation character 'B' to the CIE augmentation string
>> and the corresponding cfi directive ".cfi_b_key_frame".
> Are these extensions documented somewhere ?  And is the documentation referenced
> in the source code so that readers can find the description ?
>

There is no public documentation for this approach as it was agreed upon
internally with those implementing support in LLVM. I have documented
this in gas/doc/c-aarch64.texi, would you like me to refer to that
section from the relevant source code?

>> The pointer authentication instructions will behave as NOPs on architectures
>> that don't support them, and so a check for the architecture being assembled
>> for is not necessary since there will be no behavioural difference between
>> augmentation strings with and without the 'B' character on such architectures.
> Except that if the new .cfi_b_key_frame pseudo-op is used on an architecture
> that does not support it, the programmer might expect to see a warning message,
> yes ?  (Although reading the patch, it looks like the new pseudo-op is only
> defined for the AArch64 so other architectures cannot even use it).
>

Yes the directive will only be available when assembling for AArch64 so
it won't be usable from other architectures.

>> Built on aarch64-linux-gnu and aarch64-none-elf, and tested on aarch64-none-elf
>> with no regressions.
> Did you also test on an non AArch64 configuration, just to be sure,  eg x86_64-pc-linux-gnu ?

I haven't actually, but will do so and report back. Would just this
other configuration suffice?

> It would be nice if you also updated binutils/dwarf.c:read_cie() so that it explicitly
> handles the 'A' and 'B' encodings, even if there is nothing for it to do.  Just so
> that readers know that the encodings are recognised.  (And maybe unrecognised encodings
> should be reported....)

I agree, it would at least be good to insert a check there. I could
always add a warning when encountering unrecognised encodings in a
future patch.

> Some comments on the patch itself:
>
>> diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
>>   switch (*aug++)
>>    {
>> +  case 'B':
>> +    break;
> I would feel much happier if these encoding characters were defined once in
> a header somewhere and then #included wherever they were needed.  It is not
> necessary to make this change for this patch submission, but it would be a
> nice thing to have, if not now, then in the future.

Adding a definition of the encoding character to a header could perhaps
coincide with your suggestions below, in that I could add a macro called
"tc_is_valid_aug_char" which the target defines. That would mean we
could avoid having such an encoding definition in a target-agnostic
file. Let me know what you think.

> > diff --git a/gas/dw2gencfi.h b/gas/dw2gencfi.h
>
>> +enum pointer_auth_key {
>> +  AARCH64_PAUTH_KEY_A,
>> +  AARCH64_PAUTH_KEY_B
>> +};
> I do not like having target specific enums defined in a generic
> header file.  I would prefer to see definitions like this in
> gas/config/tc-aarch64.h.
>

Using a target macro as per your suggestions below would allow this
definition to be in tc-aarch64.h rather than in dw2gencfi.h.

>> @@ -162,6 +183,8 @@ struct fde_entry
>>     /* For out of line tables and FDEs.  */
>>     symbolS *eh_loc;
>>     int sections;
>> +  /* The pointer authentication key used.  Only used for AArch64.  */
>> +  enum pointer_auth_key pauth_key;
>>   };
> Similarly, I think that this extra field should target defined.
> Ie something like:
>
>      int sections;
>    #ifdef tc_fde_entry_extras
>      tc_fde_entry_extras
>    #endif
>    };
>
> And then define tc_fde_entry_extras in tc-aarch64.h.  This would
> also allow other backends to extend the fde_entry structure if they
> even find a need to do something similar.
>
>> diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
>> @@ -403,6 +403,7 @@ struct cie_entry
>>     unsigned char per_encoding;
>>     unsigned char lsda_encoding;
>>     expressionS personality;
>> +  enum pointer_auth_key pauth_key;
>>     struct cfi_insn_data *first, *last;
>>   };
> A similar comment applies here.
>
>> @@ -448,6 +433,7 @@ alloc_fde_entry (void)
>>     fde->per_encoding = DW_EH_PE_omit;
>>     fde->lsda_encoding = DW_EH_PE_omit;
>>     fde->eh_header_type = EH_COMPACT_UNKNOWN;
>> +  fde->pauth_key = AARCH64_PAUTH_KEY_A;
>>  
>>     return fde;
>>   }
> And of course this would have to be:
>
>       fde->eh_header_type = EH_COMPACT_UNKNOWN;
>    #ifdef tc_fde_entry_init
>       tc_fde_entry_init (fde)
>    #endif

I agree with your suggestions here, I'll get to work on them and update
with a reworked patch. In the mean time please do let me know if you
have any feedback on my comments here.

Sam
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

Ramana Radhakrishnan-7
On Fri, Nov 9, 2018 at 2:03 PM Sam Tebbs <[hidden email]> wrote:

>
> On 11/09/2018 01:27 PM, Nick Clifton wrote:
>
> > Hi Sam,
>
> Hi Nick, thanks for the feedback.
>
> >> The DWARF extensions for ARM therefore
> >> propose to add a new augmentation character 'B' to the CIE augmentation string
> >> and the corresponding cfi directive ".cfi_b_key_frame".
> > Are these extensions documented somewhere ?  And is the documentation referenced
> > in the source code so that readers can find the description ?
> >
>
> There is no public documentation for this approach as it was agreed upon
> internally with those implementing support in LLVM. I have documented
> this in gas/doc/c-aarch64.texi, would you like me to refer to that
> section from the relevant source code?

I believe this will be documented in the upcoming Dwarf supplement for AArch64.

regards
Ramana
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

Nick Clifton
In reply to this post by Sam Tebbs
Hi Sam,

>>> The DWARF extensions for ARM therefore

>> Are these extensions documented somewhere ?

> There is no public documentation for this approach as it was agreed upon
> internally with those implementing support in LLVM. I have documented
> this in gas/doc/c-aarch64.texi, would you like me to refer to that
> section from the relevant source code?

No.  I was just hoping that there would be a pdf accessible on the web
somewhere that contained a description of the exntensions to the CFI
encoding used by AArch64.  In the past ARM have been very good about
creating such documents... :-)

[Ah - just read Ramana's email, so it looks like this point is now covered.
Please could the Dwarf supplement be referenced in a comment in the revised
patch set ?]


>> Did you also test on an non AArch64 configuration, just to be sure,  eg x86_64-pc-linux-gnu ?
>
> I haven't actually, but will do so and report back. Would just this
> other configuration suffice?

Yes.  I am not actually expecting any problems.  But testing the
most popular non-ARM target configuration would be a good idea.


> Adding a definition of the encoding character to a header could perhaps
> coincide with your suggestions below, in that I could add a macro called
> "tc_is_valid_aug_char" which the target defines. That would mean we
> could avoid having such an encoding definition in a target-agnostic
> file. Let me know what you think.

The tc_xxx terminology is only used inside gas, whereas we want these characters
to be processed in lots of different places, (gas, libbfd, readelf, etc).  So I
think that you might need to put the encoding characters into a more generic
header file like include/elf/common.h or maybe include/elf/cfi.h.

Cheers
  Nick