Re: [PATCH] [1/2] arm: Implement cortex-M return signing address codegen

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 05/11/2021 08:52, Andrea Corallo via Gcc-patches wrote:
> Hi all,
> 
> this patch enables address return signature and verification based on
> Armv8.1-M Pointer Authentication [1].
> 
> To sign the return address, we use the PAC R12, LR, SP instruction
> upon function entry.  This is signing LR using SP and storing the
> result in R12.  R12 will be pushed into the stack.
> 
> During function epilogue R12 will be popped and AUT R12, LR, SP will
> be used to verify that the content of LR is still valid before return.
> 
> Here an example of PAC instrumented function prologue and epilogue:
> 
>          pac     r12, lr, sp
>          push    {r3, r7, lr}
>          push    {r12}
>          sub     sp, sp, #4

Which, as shown here, generates a stack which does not preserve 8-byte 
alignment.

Also, what's wrong with

	pac	r12, lr, sp
	push	{r3, r7, ip, lr}
?

Which saves 2 bytes in the prologue and ...

>          [...] function body
>          add     sp, sp, #4
>          pop     {r12}
>          pop     {r3, r7, lr}
>          aut     r12, lr, sp
>          bx      lr

	pop	{r3, r7, ip, lr}
	aut	r12, lr, sp
	bx	lr

which saves 4 bytes in the epilogue (repeated for each instance of the 
epilogue).

> 
> The patch also takes care of generating a PACBTI instruction in place
> of the sequence BTI+PAC when Branch Target Identification is enabled
> contextually.
> 

What about variadic functions?

What about functions where lr is live on entry (where it's used for 
passing the closure in nested functions)?


> These two patches apply on top of Tejas series posted here [2].
> 
> Regressioned and arm-linux-gnu aarch64-linux-gnu bootstraped.
> 
> Best Regards
> 
>    Andrea
> 
> [1] <https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/armv8-1-m-pointer-authentication-and-branch-target-identification-extension>
> [2] <https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581176.html>
> 


+static bool arm_pac_enabled_for_curr_function_p (void);

I really don't like that name.  There are a lot of functions with 
variations of 'current function' in the name already and this creates 
yet another variant.  Something like 
arm_current_function_pac_enabled_p() would be preferable; or, if that 
really is too long, use 'current_func' which already has usage within 
the compiler.

+(define_insn "pac_ip_lr_sp"
+  [(set (reg:DI IP_REGNUM)
+	(unspec:DI [(reg:DI SP_REGNUM) (reg:DI LR_REGNUM)]
+                   UNSPEC_PAC_IP_LR_SP))]
+  ""
+  "pac\tr12, lr, sp")
+
+(define_insn "pacbti_ip_lr_sp"
+  [(set (reg:DI IP_REGNUM)
+	(unspec:DI [(reg:DI SP_REGNUM) (reg:DI LR_REGNUM)]
+                   UNSPEC_PACBTI_IP_LR_SP))]
+  ""
+  "pacbti\tr12, lr, sp")
+
+(define_insn "aut_ip_lr_sp"
+  [(unspec:DI [(reg:DI IP_REGNUM) (reg:DI SP_REGNUM) (reg:DI LR_REGNUM)]
+              UNSPEC_AUT_IP_LR_SP)]
+  ""
+  "aut\tr12, lr, sp")
+

I think all these need a length attribute.  Also, they should only be 
enabled for thumb2 (certainly not in Arm state).
And when using explicit register names in an asm, prefix each name with 
'%|', just in case the assembler dialect has a register name prefix.

The names are somewhat unweildy, can't we use something more usefully 
descriptive, like 'pac_nop', 'pacbti_nop' and 'aut_nop', since all these 
instructions are using the architectural NOP space.

Finally, I think we need some more tests that cover the various 
frame-pointer flavours when used in combination with this feature and 
for various corners of the PCS.

R.