Discussion:
[PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key
(too old to reply)
Sam Tebbs
2018-11-01 13:23:26 UTC
Permalink
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<***@arm.com>

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

gas/
2018-11-01 Sam Tebbs<***@arm.com>

* 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<***@arm.com>

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

gas/testsuite
2018-11-01 Sam Tebbs<***@arm.com>

* gas/aarch64/(pac_ab_key.d, pac_ab_key.s): New file.
Sam Tebbs
2018-11-09 11:11:35 UTC
Permalink
Post by 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/
    * elf-eh-frame.c (_bfd_elf_parse_eh_frame): Add check for 'B'.
gas/
    * 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/
    * c-aarch64.texi (.cfi_b_key_frame): Add documentation.
gas/testsuite
    * gas/aarch64/(pac_ab_key.d, pac_ab_
Nick Clifton
2018-11-09 13:27:34 UTC
Permalink
Hi Sam,
Post by Sam Tebbs
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 ?
Post by Sam Tebbs
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).
Post by Sam Tebbs
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....)
Post by Sam Tebbs
diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
switch (*aug++)
{
+ 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.
Post by Sam Tebbs
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.
Post by Sam Tebbs
@@ -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.
Post by Sam Tebbs
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.
Post by Sam Tebbs
@@ -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
Sam Tebbs
2018-11-09 14:02:44 UTC
Permalink
Post by Nick Clifton
Hi Sam,
Hi Nick, thanks for the feedback.
Post by Nick Clifton
Post by Sam Tebbs
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?
Post by Nick Clifton
Post by Sam Tebbs
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.
Post by Nick Clifton
Post by Sam Tebbs
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?
Post by Nick Clifton
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.
Post by Nick Clifton
Post by Sam Tebbs
diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
switch (*aug++)
{
+ 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.
Post by Nick Clifton
Post by Sam Tebbs
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.
Post by Nick Clifton
Post by Sam Tebbs
@@ -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.
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.
Post by Sam Tebbs
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.
Post by Sam Tebbs
@@ -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;
}
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 co
Ramana Radhakrishnan
2018-11-09 14:17:30 UTC
Permalink
Post by Sam Tebbs
Post by Nick Clifton
Hi Sam,
Hi Nick, thanks for the feedback.
Post by Nick Clifton
Post by Sam Tebbs
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
Nick Clifton
2018-11-09 16:31:10 UTC
Permalink
Hi Sam,
Post by Sam Tebbs
Post by Nick Clifton
Post by Sam Tebbs
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 ?]
Post by Sam Tebbs
Post by Nick Clifton
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.
Post by Sam Tebbs
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
Sam Tebbs
2018-11-20 16:41:09 UTC
Permalink
Post by Nick Clifton
Hi Sam,
Post by Sam Tebbs
Post by Nick Clifton
Post by Sam Tebbs
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 ?]
I will have to follow up with the revision once it is published which
there is no concrete ETA for yet.
Post by Nick Clifton
Post by Sam Tebbs
Post by Nick Clifton
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.
I have now successfully tested and built the patch on
x86_64-pc-linux-gnu with no problems.
Post by Nick Clifton
Post by Sam Tebbs
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.
As you suggested in an earlier email, I think I will follow up with a
change to this in a later patch rather than include it in this one.

Attached is a revised patch with the target hooks implemented and
target-specific definitions (pointer_auth_key etc.).

bfd/

2018-11-20  Sam Tebbs  <***@arm.com>

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

binutils/

2018-11-20  Sam Tebbs  <***@arm.com>

    * dwarf.c (read_cie): Add check for 'B'.

gas/

2018-11-20  Sam Tebbs  <***@arm.com>

    * dw2gencfi.c (struct cie_entry): Add tc_cie_entry_extras invocation.
    (alloc_fde_entry): Add tc_fde_entry_init_extra invocation.
    (output_cie): Add tc_output_cie_extra invocation.
    (select_cie_for_fde): Add tc_cie_fde_equivalent_extra and
tc_cie_entry_init_extra
    invocation.
    (frch_cfi_data, cfa_save_data): Move to dwgencfi.h.
    * config/tc-aarch64.c (s_aarch64_cfi_b_key_frame): Declare.
    (md_pseudo_table): Add "cfi_b_key_frame".
    * config/tc-aarch64.h (tc_fde_entry_extras, tc_cie_entry_extras,
    tc_fde_entry_init_extra, tc_output_cie_extra,
tc_cie_fde_equivalent_extra,
    tc_cie_entry_init_extra): Define.
    * dw2gencfi.h (struct fde_entry): Add tc_fde_entry_extras invocation.
    (pointer_auth_key): Define.
    (frch_cfi_data, cfa_save_data): Move from dwgencfi.c.

gas/doc/

2018-11-20  Sam Tebbs  <***@arm.com>

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

gas/testsuite

2018-11-20  Sam Tebbs  <***@arm.com>

    * gas/aarch64/(pac_ab_key.d, pac_ab_key.s): New file.
Nick Clifton
2018-11-21 12:45:18 UTC
Permalink
Hi Sam,
Post by Sam Tebbs
Attached is a revised patch with the target hooks implemented and
target-specific definitions (pointer_auth_key etc.).
bfd/
    * elf-eh-frame.c (_bfd_elf_parse_eh_frame): Add check for 'B'.
binutils/
    * dwarf.c (read_cie): Add check for 'B'.
gas/
    * dw2gencfi.c (struct cie_entry): Add tc_cie_entry_extras invocation.
    (alloc_fde_entry): Add tc_fde_entry_init_extra invocation.
    (output_cie): Add tc_output_cie_extra invocation.
    (select_cie_for_fde): Add tc_cie_fde_equivalent_extra and
tc_cie_entry_init_extra
    invocation.
    (frch_cfi_data, cfa_save_data): Move to dwgencfi.h.
    * config/tc-aarch64.c (s_aarch64_cfi_b_key_frame): Declare.
    (md_pseudo_table): Add "cfi_b_key_frame".
    * config/tc-aarch64.h (tc_fde_entry_extras, tc_cie_entry_extras,
    tc_fde_entry_init_extra, tc_output_cie_extra,
tc_cie_fde_equivalent_extra,
    tc_cie_entry_init_extra): Define.
    * dw2gencfi.h (struct fde_entry): Add tc_fde_entry_extras invocation.
    (pointer_auth_key): Define.
    (frch_cfi_data, cfa_save_data): Move from dwgencfi.c.
gas/doc/
    * c-aarch64.texi (.cfi_b_key_frame): Add documentation.
Note - changelog entries for gas/doc actually go into gas/ChangeLog...
Post by Sam Tebbs
gas/testsuite
    * gas/aarch64/(pac_ab_key.d, pac_ab_key.s): New file.
Likewise - this should be in gas/ChangeLog...

Patch approved with updates to the changelog as mentioned above. Please apply.

Cheers
Nick
Sam Tebbs
2018-11-21 14:23:07 UTC
Permalink
Post by Nick Clifton
Hi Sam,
Post by Sam Tebbs
Attached is a revised patch with the target hooks implemented and
target-specific definitions (pointer_auth_key etc.).
bfd/
    * elf-eh-frame.c (_bfd_elf_parse_eh_frame): Add check for 'B'.
binutils/
    * dwarf.c (read_cie): Add check for 'B'.
gas/
    * dw2gencfi.c (struct cie_entry): Add tc_cie_entry_extras invocation.
    (alloc_fde_entry): Add tc_fde_entry_init_extra invocation.
    (output_cie): Add tc_output_cie_extra invocation.
    (select_cie_for_fde): Add tc_cie_fde_equivalent_extra and
tc_cie_entry_init_extra
    invocation.
    (frch_cfi_data, cfa_save_data): Move to dwgencfi.h.
    * config/tc-aarch64.c (s_aarch64_cfi_b_key_frame): Declare.
    (md_pseudo_table): Add "cfi_b_key_frame".
    * config/tc-aarch64.h (tc_fde_entry_extras, tc_cie_entry_extras,
    tc_fde_entry_init_extra, tc_output_cie_extra,
tc_cie_fde_equivalent_extra,
    tc_cie_entry_init_extra): Define.
    * dw2gencfi.h (struct fde_entry): Add tc_fde_entry_extras invocation.
    (pointer_auth_key): Define.
    (frch_cfi_data, cfa_save_data): Move from dwgencfi.c.
gas/doc/
    * c-aarch64.texi (.cfi_b_key_frame): Add documentation.
Note - changelog entries for gas/doc actually go into gas/ChangeLog...
Post by Sam Tebbs
gas/testsuite
    * gas/aarch64/(pac_ab_key.d, pac_ab_key.s): New file.
Likewise - this should be in gas/ChangeLog...
Ah I see. I assumed binutils used the same changelog format as GCC but
will keep this in mind. Is there a script in binutils that is similar to
GCC's contrib/mklog?
Post by Nick Clifton
Patch approved with updates to the changelog as mentioned above. Please apply.
Cheers
Nick
Thanks, I don't have commit rights so Ramana has agreed to commit it on
m

Loading...