diff mbox series

[2/4] libdw, aarch64: Implement DW_CFA_AARCH64_negate_ra_state CFI instruction

Message ID 20220425140311.95231-3-german.gomez@arm.com
State Changes Requested
Headers
Series Add AARCH64 pointer authentication support |

Commit Message

German Gomez April 25, 2022, 2:03 p.m. UTC 
  Implement DW_CFA_AARCH64_negate_ra_state in accordance with the DWARF
AARCH64 ABI [1].

Followup commits will use the value of this register to remove the PAC
from return addresses.

[1] https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#44call-frame-instructions

Signed-off-by: German Gomez <german.gomez@arm.com>
---
 libdw/cfi.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Mark Wielaard Feb. 23, 2023, 4:27 p.m. UTC | #1 
Hi German,

On Mon, 2022-04-25 at 14:03 +0000, German Gomez via Elfutils-devel
wrote:
> Implement DW_CFA_AARCH64_negate_ra_state in accordance with the DWARF
> AARCH64 ABI [1].
> 
> Followup commits will use the value of this register to remove the PAC
> from return addresses.
> 
> [1] https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#44call-frame-instructions
> 
> Signed-off-by: German Gomez <german.gomez@arm.com>

This looks good, but two comments below.

> ---
>  libdw/cfi.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/libdw/cfi.c b/libdw/cfi.c
> index a73fb03f..f985b4d8 100644
> --- a/libdw/cfi.c
> +++ b/libdw/cfi.c
> @@ -125,6 +125,15 @@ execute_cfi (Dwarf_CFI *cache,
>      fs->regs[regno].value = (r_value);			\
>    } while (0)
>  
> +  /* The AARCH64 DWARF ABI states that register 34 (ra_sign_state) must
> +     be initialized to 0. So do it before executing the CFI. */
> +  if (cache->e_machine == EM_AARCH64)
> +    {
> +      if (unlikely (! enough_registers (DW_AARCH64_RA_SIGN_STATE, &fs, &result)))
> +	goto out;
> +      fs->regs[DW_AARCH64_RA_SIGN_STATE].value = 0;
> +    }

Right. I thought this would be better expressed as part of the abi_cfi
(see aarch64_abi_cfi in backends/aarch64_cfi.c). But that would require
a DW_CFA_val_expression which we don't allow for abi_cfi. So this is
probably the best way to do it.

>    while (program < end)
>      {
>        uint8_t opcode = *program++;
> @@ -355,7 +364,10 @@ execute_cfi (Dwarf_CFI *cache,
>  	    {
>  	      /* Toggles the return address state, indicating whether
>  		 the return address is encrypted or not on
> -		 aarch64. XXX not handled yet.  */
> +		 aarch64. */
> +	      if (unlikely (! enough_registers (DW_AARCH64_RA_SIGN_STATE, &fs, &result)))
> +		goto out;
> +	      fs->regs[DW_AARCH64_RA_SIGN_STATE].value ^= 0x1;
>  	    }
>  	  else
>  	    {

Looks good.

Lets also move the DW_AARCH64_RA_SIGN_STATE definition into cfi.h (from
libdw.h in the previous patch).

Thanks,

Mark
diff mbox series

Patch

diff --git a/libdw/cfi.c b/libdw/cfi.c
index a73fb03f..f985b4d8 100644
--- a/libdw/cfi.c
+++ b/libdw/cfi.c
@@ -125,6 +125,15 @@  execute_cfi (Dwarf_CFI *cache,
     fs->regs[regno].value = (r_value);			\
   } while (0)
 
+  /* The AARCH64 DWARF ABI states that register 34 (ra_sign_state) must
+     be initialized to 0. So do it before executing the CFI. */
+  if (cache->e_machine == EM_AARCH64)
+    {
+      if (unlikely (! enough_registers (DW_AARCH64_RA_SIGN_STATE, &fs, &result)))
+	goto out;
+      fs->regs[DW_AARCH64_RA_SIGN_STATE].value = 0;
+    }
+
   while (program < end)
     {
       uint8_t opcode = *program++;
@@ -355,7 +364,10 @@  execute_cfi (Dwarf_CFI *cache,
 	    {
 	      /* Toggles the return address state, indicating whether
 		 the return address is encrypted or not on
-		 aarch64. XXX not handled yet.  */
+		 aarch64. */
+	      if (unlikely (! enough_registers (DW_AARCH64_RA_SIGN_STATE, &fs, &result)))
+		goto out;
+	      fs->regs[DW_AARCH64_RA_SIGN_STATE].value ^= 0x1;
 	    }
 	  else
 	    {