Re: [1/5][AArch64] Return address protection on AArch64

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 20/01/17 12:36, Jiong Wang wrote:
> 
> 
> On 20/01/17 11:15, Jiong Wang wrote:
>>
>>
>> On 20/01/17 03:39, Andrew Pinski wrote:
>>> On Fri, Jan 6, 2017 at 3:47 AM, Jiong Wang <jiong.wang@foss.arm.com>
>>> wrote:
>>>> On 11/11/16 18:22, Jiong Wang wrote:
>>>>> As described in the cover letter, this patch implements return address
>>>>> signing
>>>>> for AArch64, it's controlled by the new option:
>>>>>
>>>>>     -msign-return-address=[none | non-leaf | all]
>>>>>
>>>>> "none" means don't do return address signing at all on any function.
>>>>> "non-leaf"
>>>>> means only sign non-leaf function.  "all" means sign all functions.
>>>>> Return
>>>>> address signing is currently disabled on ILP32.  I haven't tested it.
>>>>>
>>>>> The instructions added in the architecture are of 2 kinds.
>>>>>
>>>>> * In the NOP instruction space, which allows binaries to run
>>>>> without any
>>>>> traps
>>>>> on older versions of the architecture. This doesn't give any
>>>>> additional
>>>>> protection on older hardware but allows for the same binary to be
>>>>> used on
>>>>> earlier versions of the architecture and newer versions of the
>>>>> architecture.
>>>>>
>>>>> * New instructions that are only valid for v8.3 and will trap if
>>>>> used on
>>>>> earlier
>>>>> versions of the architecture.
>>>>>
>>>>> At default, once return address signing is enabled, it will only
>>>>> generates
>>>>> NOP
>>>>> instruction.
>>>>>
>>>>> While if -march=armv8.3-a specified, GCC will try to use the most
>>>>> efficient
>>>>> pointer authentication instruction as it can.
>>>>>
>>>>> The architecture has 2 user invisible system keys for signing and
>>>>> creating
>>>>> signed addresses as part of these instructions. For some use case, the
>>>>> user
>>>>> might want to use difference key for different functions. The new
>>>>> option
>>>>> "-msign-return-address-key=key_name" let GCC select the key used for
>>>>> return
>>>>> address signing.  Permissible values are "a_key" for A key and
>>>>> "b_key" for
>>>>> B
>>>>> key, and this option are supported by function target attribute and
>>>>> LTO
>>>>> will
>>>>> hopefully just work.
>>>>>
>>>>>
>>>>>
>>>>> gcc/
>>>>> 2016-11-09  Jiong Wang<jiong.wang@arm.com>
>>>>>
>>>>>           * config/aarch64/aarch64-opts.h
>>>>> (aarch64_pauth_key_index): New
>>>>> enum.
>>>>>           (aarch64_function_type): New enum.
>>>>>           * config/aarch64/aarch64-protos.h
>>>>> (aarch64_output_sign_auth_reg):
>>>>> New
>>>>>           declaration.
>>>>>           * config/aarch64/aarch64.c (aarch64_expand_prologue):
>>>>> Sign return
>>>>>           address before it's pushed onto stack.
>>>>>           (aarch64_expand_epilogue): Authenticate return address
>>>>> fetched
>>>>> from
>>>>>           stack.
>>>>>           (aarch64_output_sign_auth_reg): New function.
>>>>>           (aarch64_override_options): Sanity check for ILP32 and
>>>>> ISA level.
>>>>>           (aarch64_attributes): New function attributes for
>>>>> "sign-return-address",
>>>>>           "pauth-key".
>>>>>           * config/aarch64/aarch64.md (UNSPEC_AUTH_REG,
>>>>> UNSPEC_AUTH_REG1716,
>>>>>           UNSPEC_SIGN_REG, UNSPEC_SIGN_REG1716, UNSPEC_STRIP_REG_SIGN,
>>>>>           UNSPEC_STRIP_X30_SIGN): New unspecs.
>>>>>           ("*do_return"): Generate combined instructions according
>>>>> to key
>>>>> index.
>>>>>           ("sign_reg", "sign_reg1716", "auth_reg", "auth_reg1716",
>>>>>           "strip_reg_sign", "strip_lr_sign"): New.
>>>>>           * config/aarch64/aarch64.opt (msign-return-address,
>>>>> mpauth-key):
>>>>> New.
>>>>>           * config/aarch64/predicates.md (aarch64_const0_const1): New
>>>>> predicate.
>>>>>           * doc/extend.texi (AArch64 Function Attributes): Documents
>>>>>           "sign-return-address=", "pauth-key".
>>>>>           * doc/invoke.texi (AArch64 Options): Documents
>>>>> "-msign-return-address=",
>>>>>           "-pauth-key".
>>>>>
>>>>> gcc/testsuite/
>>>>> 2016-11-09  Jiong Wang<jiong.wang@arm.com>
>>>>>
>>>>>           * gcc.target/aarch64/return_address_sign_1.c: New testcase.
>>>>>           * gcc.target/aarch64/return_address_sign_scope_1.c: New
>>>>> testcase.
>>>>
>>>> Update the patchset according to new DWARF proposal described at
>>>>
>>>>    https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03010.html
>>> One of these patches of this patch set break ILP32 building for
>>> aarch64-elf and most likely also aarch64-linux-gnu.
>>>
>>> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/unwind-dw2.c:
>>>
>>> In function â??uw_init_context_1â??:
>>> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/unwind-dw2.c:1567:6:
>>>
>>> internal compiler error: in emit_move_insn, at expr.c:3698
>>>     ra = MD_POST_EXTRACT_ROOT_ADDR (ra);
>>> 0x8270cf emit_move_insn(rtx_def*, rtx_def*)
>>> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/expr.c:3697
>>>
>>> 0x80867b force_reg(machine_mode, rtx_def*)
>> Must be the Pmode issue under ILP32, I am testing a fix (I don't have
>> full ILP32 environment, so can only test simply by force libgcc build
>> with -mabi=ilp32)
> 
> Here is the patch.
> 
> For XPACLRI builtin which drops the signature in a pointer, it's
> prototype is  "void *foo (void *)"
> FOR PAC/AUT builtin which sign or authenticate a pointer, it's prototype
> is "void *foo (void *, uint64)".
> 
> This patch adjusted those modes to make sure they strictly follow the C
> prototype. I also borrow the type define in ARM backend
> 
>   typedef unsigned _uw64 __attribute__((mode(__DI__)));
> 
> And this is need to type cast the salt value which is always DImode.
> 
> It passed my local ILP32 cross build.
> 
> OK for trunk?
> 
> gcc/
> 2017-01-20  Jiong Wang  <jiong.wang@arm.com>
>         * config/aarch64/aarch64-builtins.c (aarch64_expand_builtin):
> Fix modes
>         for AARCH64_PAUTH_BUILTIN_XPACLRI,
> AARCH64_PAUTH_BUILTIN_PACIA1716 and
>         AARCH64_PAUTH_BUILTIN_AUTIA1716.
> 
> libgcc/
>         * config/aarch64/aarch64-unwind.h (_uw64): New typedef.
>         (aarch64_post_extract_frame_addr):  Cast salt to _uw64.
>         (aarch64_post_frob_eh_handler_addr): Likewise.
> 
> 

Hmm, we currently don't support ILP32 at all for pointer signing (sorry
("Return address signing is only supported for -mabi=lp64");), so I
wonder if it would be better for now to simply suppress all the new
hooks in aarch64-unwind.h ifdef __ILP32__.

R.

> k.patch
> 
> 
> diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
> index 6c6530c..7ef351e 100644
> --- a/gcc/config/aarch64/aarch64-builtins.c
> +++ b/gcc/config/aarch64/aarch64-builtins.c
> @@ -1333,18 +1333,20 @@ aarch64_expand_builtin (tree exp,
>      case AARCH64_PAUTH_BUILTIN_PACIA1716:
>      case AARCH64_PAUTH_BUILTIN_XPACLRI:
>        arg0 = CALL_EXPR_ARG (exp, 0);
> -      op0 = force_reg (Pmode, expand_normal (arg0));
> +      /* Operand0 should have ptr_mode as its a ptr_type_node, this makes both
> +	 LP64 and ILP32 expand correctly.  */
> +      op0 = force_reg (ptr_mode, expand_normal (arg0));
>  
>        if (!target)
> -	target = gen_reg_rtx (Pmode);
> +	target = gen_reg_rtx (ptr_mode);
>        else
> -	target = force_reg (Pmode, target);
> +	target = force_reg (ptr_mode, target);
>  
>        emit_move_insn (target, op0);
>  
>        if (fcode == AARCH64_PAUTH_BUILTIN_XPACLRI)
>  	{
> -	  rtx lr = gen_rtx_REG (Pmode, R30_REGNUM);
> +	  rtx lr = gen_rtx_REG (ptr_mode, R30_REGNUM);
>  	  icode = CODE_FOR_xpaclri;
>  	  emit_move_insn (lr, op0);
>  	  emit_insn (GEN_FCN (icode) ());
> @@ -1353,12 +1355,13 @@ aarch64_expand_builtin (tree exp,
>        else
>  	{
>  	  tree arg1 = CALL_EXPR_ARG (exp, 1);
> -	  rtx op1 = force_reg (Pmode, expand_normal (arg1));
> +	  /* Operand1 for either PAC or AUT is a unsigned_intDI_type_node.  */
> +	  rtx op1 = force_reg (DImode, expand_normal (arg1));
>  	  icode = (fcode == AARCH64_PAUTH_BUILTIN_PACIA1716
>  		   ? CODE_FOR_paci1716 : CODE_FOR_auti1716);
>  
> -	  rtx x16_reg = gen_rtx_REG (Pmode, R16_REGNUM);
> -	  rtx x17_reg = gen_rtx_REG (Pmode, R17_REGNUM);
> +	  rtx x16_reg = gen_rtx_REG (DImode, R16_REGNUM);
> +	  rtx x17_reg = gen_rtx_REG (ptr_mode, R17_REGNUM);
>  	  emit_move_insn (x17_reg, op0);
>  	  emit_move_insn (x16_reg, op1);
>  	  emit_insn (GEN_FCN (icode) ());
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index a43d965..7dd2549 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -35,6 +35,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  #define MD_FROB_UPDATE_CONTEXT(context, fs) \
>    aarch64_frob_update_context (context, fs)
>  
> +typedef unsigned _uw64 __attribute__((mode(__DI__)));
> +
>  /* Do AArch64 private extraction on ADDR based on context info CONTEXT and
>     unwind frame info FS.  If ADDR is signed, we do address authentication on it
>     using CFA of current frame.  */
> @@ -45,7 +47,7 @@ aarch64_post_extract_frame_addr (struct _Unwind_Context *context,
>  {
>    if (fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset & 0x1)
>      {
> -      _Unwind_Word salt = (_Unwind_Word) context->cfa;
> +      _uw64 salt = (_uw64) context->cfa;
>        return __builtin_aarch64_autia1716 (addr, salt);
>      }
>    else
> @@ -64,7 +66,7 @@ aarch64_post_frob_eh_handler_addr (struct _Unwind_Context *current,
>  {
>    if (current->flags & RA_A_SIGNED_BIT)
>      return __builtin_aarch64_pacia1716 (handler_addr,
> -					(_Unwind_Word) current->cfa);
> +					(_uw64) current->cfa);
>    else
>      return handler_addr;
>  }
>