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

[Sam Tebbs <Sam dot Tebbs at arm dot com>]

On 11/21/18 12:45 PM, Nick Clifton wrote:

> Hi Sam,
>
>> Attached is a revised patch with the target hooks implemented and
>> target-specific definitions (pointer_auth_key etc.).
>>
>> bfd/
>>
>> 2018-11-20  Sam Tebbs  <sam.tebbs@arm.com>
>>
>>       * elf-eh-frame.c (_bfd_elf_parse_eh_frame): Add check for 'B'.
>>
>> binutils/
>>
>> 2018-11-20  Sam Tebbs  <sam.tebbs@arm.com>
>>
>>       * dwarf.c (read_cie): Add check for 'B'.
>>
>> gas/
>>
>> 2018-11-20  Sam Tebbs  <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  <sam.tebbs@arm.com>
>>
>>       * c-aarch64.texi (.cfi_b_key_frame): Add documentation.
> Note - changelog entries for gas/doc actually go into gas/ChangeLog...
>
>> gas/testsuite
>>
>> 2018-11-20  Sam Tebbs  <sam.tebbs@arm.com>
>>
>>       * 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
>
Made a mistake, the patch I wanted you to review is FOO. I've also 
attached the diff between the one you reviewed and FOO for you to see if 
you are happy for me to commit the difference. Below is a changelog for 
this patch.

binutils/ChangeLog:

2018-12-11  Sam Tebbs  <sam.tebbs@arm.com>

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

gas/ChangeLog:

2018-12-11  Sam Tebbs  <sam.tebbs@arm.com>

     * config/tc-aarch64.h (enum pointer_auth_key,
     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.c (struct cie_entry): Add tc_cie_entry_extras invocation.
     (alloc_fde_entry, select_cie_for_fde): 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 invocation.
     * dw2gencfi.h (enum pointer_auth_key): Move to config/tc-aarch64.h.
     (struct fde_entry): Add tc_fde_entry_extras invocation

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index a85a9ab..e786bc4 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -7401,6 +7401,8 @@ read_cie (unsigned char *start, unsigned char *end,
 	    fc->fde_encoding = *q++;
 	  else if (*p == 'S')
 	    ;
+	  else if (*p == 'B')
+	    ;
 	  else
 	    break;
 	  p++;
diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 88df2aa..0fd89c8 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -80,6 +80,38 @@ struct aarch64_fix
 
 #define tc_frob_section(S) aarch64_frob_section (S)
 
+/* The key used to sign a function's return address.  */
+enum pointer_auth_key {
+  AARCH64_PAUTH_KEY_A,
+  AARCH64_PAUTH_KEY_B
+};
+
+/* The extra fields required by AArch64 in fde_entry and cie_entry.  Currently
+   only used to store the key used to sign the frame's return address.  */
+#define tc_fde_entry_extras enum pointer_auth_key pauth_key;
+#define tc_cie_entry_extras enum pointer_auth_key pauth_key;
+
+/* The extra initialisation steps needed by AArch64 in alloc_fde_entry.
+   Currently only used to initialise the key used to sign the return
+   address.  */
+#define tc_fde_entry_init_extra(fde) fde->pauth_key = AARCH64_PAUTH_KEY_A;
+
+/* Extra checks required by AArch64 when outputting the current cie_entry.
+   Currently only used to output a 'B' if the return address is signed with the
+   B key.  */
+#define tc_output_cie_extra(cie) if (cie->pauth_key == AARCH64_PAUTH_KEY_B) \
+			      out_one ('B');
+
+/* Extra equivalence checks required by AArch64 when selecting the correct cie
+   for some fde.  Currently only used to check for quivalence between keys used
+   to sign ther return address.  */
+#define tc_cie_fde_equivalent_extra(cie, fde) (cie->pauth_key == fde->pauth_key)
+
+/* The extra initialisation steps needed by AArch64 in select_cie_for_fde.
+   Currently only used to initialise the key used to sign the return
+   address.  */
+#define tc_cie_entry_init_extra(cie, fde) cie->pauth_key = fde->pauth_key;
+
 #define TC_FIX_TYPE struct aarch64_fix
 #define TC_INIT_FIX_DATA(FIX) { (FIX)->tc_fix_data.inst = NULL;	\
     (FIX)->tc_fix_data.opnd = AARCH64_OPND_NIL; }
diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
index ff5c0df..cb4c50b 100644
--- a/gas/dw2gencfi.c
+++ b/gas/dw2gencfi.c
@@ -403,7 +403,9 @@ struct cie_entry
   unsigned char per_encoding;
   unsigned char lsda_encoding;
   expressionS personality;
-  enum pointer_auth_key pauth_key;
+#ifdef tc_cie_entry_extras
+  tc_cie_entry_extras
+#endif
   struct cfi_insn_data *first, *last;
 };
 
@@ -433,7 +435,9 @@ 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;
+#ifdef tc_fde_entry_init_extra
+  tc_fde_entry_init_extra (fde)
+#endif
 
   return fde;
 }
@@ -1575,7 +1579,7 @@ dot_cfi_personality_id (int ignored ATTRIBUTE_UNUSED)
   ignore_rest_of_line ();
 }
 #endif
-
+
 static void
 output_cfi_insn (struct cfi_insn_data *insn)
 {
@@ -1858,8 +1862,9 @@ output_cie (struct cie_entry *cie, bfd_boolean eh_frame, int align)
       if (cie->lsda_encoding != DW_EH_PE_omit)
 	out_one ('L');
       out_one ('R');
-      if (cie->pauth_key == AARCH64_PAUTH_KEY_B)
-	out_one ('B');
+#ifdef tc_output_cie_extra
+      tc_output_cie_extra (cie)
+#endif
     }
   if (cie->signal_frame)
     out_one ('S');
@@ -2039,8 +2044,11 @@ select_cie_for_fde (struct fde_entry *fde, bfd_boolean eh_frame,
     {
       if (CUR_SEG (cie) != CUR_SEG (fde))
 	continue;
+#ifdef tc_cie_fde_equivalent_extra
+      if (!tc_cie_fde_equivalent_extra (cie, fde))
+	continue;
+#endif
       if (cie->return_column != fde->return_column
-	  || cie->pauth_key != fde->pauth_key
 	  || cie->signal_frame != fde->signal_frame
 	  || cie->per_encoding != fde->per_encoding
 	  || cie->lsda_encoding != fde->lsda_encoding)
@@ -2147,7 +2155,9 @@ select_cie_for_fde (struct fde_entry *fde, bfd_boolean eh_frame,
   cie->lsda_encoding = fde->lsda_encoding;
   cie->personality = fde->personality;
   cie->first = fde->data;
-  cie->pauth_key = fde->pauth_key;
+#ifdef tc_cie_entry_init_extra
+  tc_cie_entry_init_extra (cie, fde)
+#endif
 
   for (i = cie->first; i ; i = i->next)
     if (i->insn == DW_CFA_advance_loc
diff --git a/gas/dw2gencfi.h b/gas/dw2gencfi.h
index 2b1362a..308032f 100644
--- a/gas/dw2gencfi.h
+++ b/gas/dw2gencfi.h
@@ -135,11 +135,6 @@ enum {
   EH_COMPACT_HAS_LSDA
 };
 
-enum pointer_auth_key {
-  AARCH64_PAUTH_KEY_A,
-  AARCH64_PAUTH_KEY_B
-};
-
 /* Stack of old CFI data, for save/restore.  */
 struct cfa_save_data
 {
@@ -183,8 +178,9 @@ 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;
+#ifdef tc_fde_entry_extras
+  tc_fde_entry_extras
+#endif
 };
 
 /* The list of all FDEs that have been collected.  */