diff mbox series

[v1,3/4] aarch64 DWARF: add new CFI directive for PAuth_LR

Message ID 20241125162846.94691-4-matthieu.longo@arm.com
State New
Headers
Series aarch64: add DWARF and SFrame support for new CFI directive used for PAuth_LR |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Matthieu Longo Nov. 25, 2024, 4:28 p.m. UTC 
  This patch adds a new CFI directive (cfi_negate_ra_state_with_pc) which
set an additional bit in the RA state to inform that RA was signed with
SP but also PC as an additional diversifier.

RA state | Description
0b00     | Return address not signed (default if no cfi_negate_ra_state*)
0b01     | Return address signed with SP (cfi_negate_ra_state)
0b10     | Invalid state
0b11     | Return address signed with SP+PC (cfi_negate_ra_state_with_pc)
---
 bfd/elf-eh-frame.c |  1 +
 binutils/dwarf.c   |  5 +++++
 gas/dw2gencfi.c    | 10 ++++++++++
 gas/scfidw2gen.c   |  1 +
 include/dwarf2.def |  4 +++-
 5 files changed, 20 insertions(+), 1 deletion(-)

Comments

Jan Beulich Nov. 25, 2024, 4:40 p.m. UTC | #1 
On 25.11.2024 17:28, Matthieu Longo wrote:
> --- a/include/dwarf2.def
> +++ b/include/dwarf2.def
> @@ -788,9 +788,11 @@ DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
>  /* GNU extensions.
>     NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>  DW_CFA (DW_CFA_GNU_window_save, 0x2d)
> -DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>  DW_CFA (DW_CFA_GNU_args_size, 0x2e)
>  DW_CFA (DW_CFA_GNU_negative_offset_extended, 0x2f)
> +/* AArch64 extensions. */
> +DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
> +DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)

While I understand an important aspect here is to stay in sync with the
gcc incarnation of this file, I don't think it's a good idea to re-order
things. Instead 0x2c should go further up, such that all of these entries
remain numerically sorted. Only then will the 2nd line of the comment
actually remain directly next to _both_ values it applies to.

Jan
Jan Beulich Nov. 25, 2024, 4:42 p.m. UTC | #2 
On 25.11.2024 17:28, Matthieu Longo wrote:
> --- a/gas/dw2gencfi.c
> +++ b/gas/dw2gencfi.c
> @@ -718,6 +718,7 @@ const pseudo_typeS cfi_pseudo_table[] =
>      { "cfi_restore_state", dot_cfi, DW_CFA_restore_state },
>      { "cfi_window_save", dot_cfi, DW_CFA_GNU_window_save },
>      { "cfi_negate_ra_state", dot_cfi, DW_CFA_AARCH64_negate_ra_state },
> +    { "cfi_negate_ra_state_with_pc", dot_cfi, DW_CFA_AARCH64_negate_ra_state_with_pc },
>      { "cfi_escape", dot_cfi_escape, 0 },
>      { "cfi_signal_frame", dot_cfi, CFI_signal_frame },
>      { "cfi_personality", dot_cfi_personality, 0 },
> @@ -918,6 +919,10 @@ dot_cfi (int arg)
>        cfi_add_CFA_insn (DW_CFA_GNU_window_save);
>        break;
>  
> +    case DW_CFA_AARCH64_negate_ra_state_with_pc:
> +      cfi_add_CFA_insn (DW_CFA_AARCH64_negate_ra_state_with_pc);
> +      break;

Oh, and: Shouldn't we learn from the confusion about the two other,
aliasing directives? IOW - what if this new directive is used on other
than Aarch64? Imo that would better be refused, to avoid ending up with
a similar problem again.

Jan
Matthieu Longo Nov. 25, 2024, 5:33 p.m. UTC | #3 
On 2024-11-25 16:40, Jan Beulich wrote:
> On 25.11.2024 17:28, Matthieu Longo wrote:
>> --- a/include/dwarf2.def
>> +++ b/include/dwarf2.def
>> @@ -788,9 +788,11 @@ DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
>>   /* GNU extensions.
>>      NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>>   DW_CFA (DW_CFA_GNU_window_save, 0x2d)
>> -DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>>   DW_CFA (DW_CFA_GNU_args_size, 0x2e)
>>   DW_CFA (DW_CFA_GNU_negative_offset_extended, 0x2f)
>> +/* AArch64 extensions. */
>> +DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
>> +DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
> 
> While I understand an important aspect here is to stay in sync with the
> gcc incarnation of this file, I don't think it's a good idea to re-order
> things. Instead 0x2c should go further up, such that all of these entries
> remain numerically sorted. Only then will the 2nd line of the comment
> actually remain directly next to _both_ values it applies to.
> 
> Jan

The numeric sorting does not seem to be always respected when it makes 
sense to group the entries.
For instance, line 783, a few line above:

  DW_CFA (DW_CFA_lo_user, 0x1c)
  DW_CFA (DW_CFA_hi_user, 0x3f)

I can do as you proposed, I could move the AArch64 extensions between 
the SGI/MIPS entries and GNU ones.
Something like:

   /* SGI/MIPS specific.  */
   DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
   /* AArch64 extensions. */
   DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
   DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
   /* GNU extensions.
    NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
   DW_CFA (DW_CFA_GNU_window_save, 0x2d)
   DW_CFA (DW_CFA_GNU_args_size, 0x2e)

Do you think it looks better ?
Matthieu Longo Nov. 25, 2024, 5:37 p.m. UTC | #4 
On 2024-11-25 16:42, Jan Beulich wrote:
> On 25.11.2024 17:28, Matthieu Longo wrote:
>> --- a/gas/dw2gencfi.c
>> +++ b/gas/dw2gencfi.c
>> @@ -718,6 +718,7 @@ const pseudo_typeS cfi_pseudo_table[] =
>>       { "cfi_restore_state", dot_cfi, DW_CFA_restore_state },
>>       { "cfi_window_save", dot_cfi, DW_CFA_GNU_window_save },
>>       { "cfi_negate_ra_state", dot_cfi, DW_CFA_AARCH64_negate_ra_state },
>> +    { "cfi_negate_ra_state_with_pc", dot_cfi, DW_CFA_AARCH64_negate_ra_state_with_pc },
>>       { "cfi_escape", dot_cfi_escape, 0 },
>>       { "cfi_signal_frame", dot_cfi, CFI_signal_frame },
>>       { "cfi_personality", dot_cfi_personality, 0 },
>> @@ -918,6 +919,10 @@ dot_cfi (int arg)
>>         cfi_add_CFA_insn (DW_CFA_GNU_window_save);
>>         break;
>>   
>> +    case DW_CFA_AARCH64_negate_ra_state_with_pc:
>> +      cfi_add_CFA_insn (DW_CFA_AARCH64_negate_ra_state_with_pc);
>> +      break;
> 
> Oh, and: Shouldn't we learn from the confusion about the two other,
> aliasing directives? IOW - what if this new directive is used on other
> than Aarch64? Imo that would better be refused, to avoid ending up with
> a similar problem again.
> 
> Jan

I agree with you, that would be the best approach.
That's what I did recently in GCC by moving architecture extension to 
the backend: 
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=9e1c71bab50d51a1a8ec1a75080ffde6ca3d854c
Unfortunately, from my understanding, this requires a major refactoring 
of the current implementation.
Jan Beulich Nov. 26, 2024, 7:20 a.m. UTC | #5 
On 25.11.2024 18:37, Matthieu Longo wrote:
> On 2024-11-25 16:42, Jan Beulich wrote:
>> On 25.11.2024 17:28, Matthieu Longo wrote:
>>> --- a/gas/dw2gencfi.c
>>> +++ b/gas/dw2gencfi.c
>>> @@ -718,6 +718,7 @@ const pseudo_typeS cfi_pseudo_table[] =
>>>       { "cfi_restore_state", dot_cfi, DW_CFA_restore_state },
>>>       { "cfi_window_save", dot_cfi, DW_CFA_GNU_window_save },
>>>       { "cfi_negate_ra_state", dot_cfi, DW_CFA_AARCH64_negate_ra_state },
>>> +    { "cfi_negate_ra_state_with_pc", dot_cfi, DW_CFA_AARCH64_negate_ra_state_with_pc },
>>>       { "cfi_escape", dot_cfi_escape, 0 },
>>>       { "cfi_signal_frame", dot_cfi, CFI_signal_frame },
>>>       { "cfi_personality", dot_cfi_personality, 0 },
>>> @@ -918,6 +919,10 @@ dot_cfi (int arg)
>>>         cfi_add_CFA_insn (DW_CFA_GNU_window_save);
>>>         break;
>>>   
>>> +    case DW_CFA_AARCH64_negate_ra_state_with_pc:
>>> +      cfi_add_CFA_insn (DW_CFA_AARCH64_negate_ra_state_with_pc);
>>> +      break;
>>
>> Oh, and: Shouldn't we learn from the confusion about the two other,
>> aliasing directives? IOW - what if this new directive is used on other
>> than Aarch64? Imo that would better be refused, to avoid ending up with
>> a similar problem again.
> 
> I agree with you, that would be the best approach.
> That's what I did recently in GCC by moving architecture extension to 
> the backend: 
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=9e1c71bab50d51a1a8ec1a75080ffde6ca3d854c
> Unfortunately, from my understanding, this requires a major refactoring 
> of the current implementation.

Well, wouldn't a simple "#ifdef TC_AARCH64" do?

Jan
Jan Beulich Nov. 26, 2024, 7:22 a.m. UTC | #6 
On 25.11.2024 18:33, Matthieu Longo wrote:
> On 2024-11-25 16:40, Jan Beulich wrote:
>> On 25.11.2024 17:28, Matthieu Longo wrote:
>>> --- a/include/dwarf2.def
>>> +++ b/include/dwarf2.def
>>> @@ -788,9 +788,11 @@ DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
>>>   /* GNU extensions.
>>>      NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>>>   DW_CFA (DW_CFA_GNU_window_save, 0x2d)
>>> -DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>>>   DW_CFA (DW_CFA_GNU_args_size, 0x2e)
>>>   DW_CFA (DW_CFA_GNU_negative_offset_extended, 0x2f)
>>> +/* AArch64 extensions. */
>>> +DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
>>> +DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>>
>> While I understand an important aspect here is to stay in sync with the
>> gcc incarnation of this file, I don't think it's a good idea to re-order
>> things. Instead 0x2c should go further up, such that all of these entries
>> remain numerically sorted. Only then will the 2nd line of the comment
>> actually remain directly next to _both_ values it applies to.
>>
>> Jan
> 
> The numeric sorting does not seem to be always respected when it makes 
> sense to group the entries.
> For instance, line 783, a few line above:
> 
>   DW_CFA (DW_CFA_lo_user, 0x1c)
>   DW_CFA (DW_CFA_hi_user, 0x3f)
> 
> I can do as you proposed, I could move the AArch64 extensions between 
> the SGI/MIPS entries and GNU ones.
> Something like:
> 
>    /* SGI/MIPS specific.  */
>    DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
>    /* AArch64 extensions. */
>    DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
>    DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>    /* GNU extensions.
>     NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>    DW_CFA (DW_CFA_GNU_window_save, 0x2d)
>    DW_CFA (DW_CFA_GNU_args_size, 0x2e)
> 
> Do you think it looks better ?

Not necessarily, plus it would get worse when more extensions are added
like this.

   /* SGI/MIPS specific.  */
   DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
   /* AArch64 specific. */
   DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
   /* GNU extensions.
    NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
   DW_CFA (DW_CFA_GNU_window_save, 0x2d)
   DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
   DW_CFA (DW_CFA_GNU_args_size, 0x2e)

IOW personally I consider it particularly relevant that the DW_CFA_DUP()
stay immediately next to (after) its corresponding DW_CFA().

Jan
diff mbox series

Patch

diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
index ebe162f2e5f..6029ae75a26 100644
--- a/bfd/elf-eh-frame.c
+++ b/bfd/elf-eh-frame.c
@@ -359,6 +359,7 @@  skip_cfa_op (bfd_byte **iter, bfd_byte *end, unsigned int encoded_ptr_width)
     case DW_CFA_remember_state:
     case DW_CFA_restore_state:
     case DW_CFA_GNU_window_save:
+    case DW_CFA_AARCH64_negate_ra_state_with_pc:
       /* No arguments.  */
       return true;
 
diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 4b46100c753..f02f4ba6fd2 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -10355,6 +10355,11 @@  display_debug_frames (struct dwarf_section *section,
 	      fc->pc_begin += ofs;
 	      break;
 
+	    case DW_CFA_AARCH64_negate_ra_state_with_pc:
+	      if (! do_debug_frames_interp)
+		printf ("  DW_CFA_AARCH64_negate_ra_state_with_pc\n");
+	      break;
+
 	    case DW_CFA_GNU_window_save:
 	      if (! do_debug_frames_interp)
 		printf ("  %s\n", DW_CFA_GNU_window_save_name[is_aarch64]);
diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
index 14b73ef33c5..5071a161576 100644
--- a/gas/dw2gencfi.c
+++ b/gas/dw2gencfi.c
@@ -718,6 +718,7 @@  const pseudo_typeS cfi_pseudo_table[] =
     { "cfi_restore_state", dot_cfi, DW_CFA_restore_state },
     { "cfi_window_save", dot_cfi, DW_CFA_GNU_window_save },
     { "cfi_negate_ra_state", dot_cfi, DW_CFA_AARCH64_negate_ra_state },
+    { "cfi_negate_ra_state_with_pc", dot_cfi, DW_CFA_AARCH64_negate_ra_state_with_pc },
     { "cfi_escape", dot_cfi_escape, 0 },
     { "cfi_signal_frame", dot_cfi, CFI_signal_frame },
     { "cfi_personality", dot_cfi_personality, 0 },
@@ -918,6 +919,10 @@  dot_cfi (int arg)
       cfi_add_CFA_insn (DW_CFA_GNU_window_save);
       break;
 
+    case DW_CFA_AARCH64_negate_ra_state_with_pc:
+      cfi_add_CFA_insn (DW_CFA_AARCH64_negate_ra_state_with_pc);
+      break;
+
     case CFI_signal_frame:
       frchain_now->frch_cfi_data->cur_fde_data->signal_frame = 1;
       break;
@@ -1758,6 +1763,10 @@  output_cfi_insn (struct cfi_insn_data *insn)
       out_one (DW_CFA_GNU_window_save);
       break;
 
+    case DW_CFA_AARCH64_negate_ra_state_with_pc:
+      out_one (DW_CFA_AARCH64_negate_ra_state_with_pc);
+      break;
+
     case CFI_escape:
       {
 	struct cfi_escape_data *e;
@@ -2216,6 +2225,7 @@  cfi_change_reg_numbers (struct cfi_insn_data *insn, segT ccseg)
 	case DW_CFA_remember_state:
 	case DW_CFA_restore_state:
 	case DW_CFA_GNU_window_save:
+	case DW_CFA_AARCH64_negate_ra_state_with_pc:
 	case CFI_escape:
 	case CFI_label:
 	  break;
diff --git a/gas/scfidw2gen.c b/gas/scfidw2gen.c
index 2b018fac8bd..1fd0cd832e5 100644
--- a/gas/scfidw2gen.c
+++ b/gas/scfidw2gen.c
@@ -113,6 +113,7 @@  const pseudo_typeS scfi_pseudo_table[] =
     { "cfi_restore_state", dot_scfi_ignore, 0 },
     { "cfi_window_save", dot_scfi_ignore, 0 },
     { "cfi_negate_ra_state", dot_scfi_ignore, 0 },
+    { "cfi_negate_ra_state_with_pc", dot_scfi_ignore, 0 },
     { "cfi_escape", dot_scfi_ignore, 0 },
     { "cfi_personality", dot_scfi_ignore, 0 },
     { "cfi_personality_id", dot_scfi_ignore, 0 },
diff --git a/include/dwarf2.def b/include/dwarf2.def
index 66c7fa1220f..95601fc7018 100644
--- a/include/dwarf2.def
+++ b/include/dwarf2.def
@@ -788,9 +788,11 @@  DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
 /* GNU extensions.
    NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
 DW_CFA (DW_CFA_GNU_window_save, 0x2d)
-DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
 DW_CFA (DW_CFA_GNU_args_size, 0x2e)
 DW_CFA (DW_CFA_GNU_negative_offset_extended, 0x2f)
+/* AArch64 extensions. */
+DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
+DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
 
 DW_END_CFA