Re: [PATCH 10/12] aarch64: Add pac-ret support to asm files

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 30/04/2020 14:44, Szabolcs Nagy wrote:
> From de8968ed58686c26391de8343184a1283bb5e305 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Date: Wed, 29 Apr 2020 11:49:20 +0100
> Subject: [PATCH 10/12] aarch64: Add pac-ret support to asm files
> 
> This patch unconditionally enables pac-ret in asm files.
> 
> TODO: This will need configure checks, cannot be done
> unconditionally because we cannot guarantee pac-ret
> compatibility (e.g. libgcc unwinder had no support for
> it before gcc-7 and newer libgcc had bugs that could
> cause unwind crash when pac-ret and non-pac-ret stack
> frames are mixed)

Which gcc version does it work correctly? Would it be a check against
a specific gcc version or could it be a configure to see if libgcc
provides the expected fixes? 

The gcc-7 branch is now closed, so maybe we could assume gcc-8 and 
have the required fixes backported?

In any case I think we should add a configure check based on compiler
-mbranch-protection= options used to enable/disable ENABLE_PAC_RET.


> ---
>  sysdeps/aarch64/crti.S          |  8 ++++++++
>  sysdeps/aarch64/crtn.S          |  6 ++++++
>  sysdeps/aarch64/dl-tlsdesc.S    |  8 ++++++++
>  sysdeps/aarch64/dl-trampoline.S | 15 ++++++++++++++-
>  sysdeps/aarch64/sysdep.h        | 18 +++++++++++++++++-
>  5 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/aarch64/crti.S b/sysdeps/aarch64/crti.S
> index 89a9e25f5b..36f58c9a01 100644
> --- a/sysdeps/aarch64/crti.S
> +++ b/sysdeps/aarch64/crti.S
> @@ -75,7 +75,11 @@ call_weak_fn:
>  	.hidden	_init
>  	.type	_init, %function
>  _init:
> +#if ENABLE_PAC_RET
> +	PACIASP
> +#else
>  	BTI_C
> +#endif
>  	stp	x29, x30, [sp, -16]!
>  	mov	x29, sp
>  #if PREINIT_FUNCTION_WEAK
> @@ -90,7 +94,11 @@ _init:
>  	.hidden	_fini
>  	.type	_fini, %function
>  _fini:
> +#if ENABLE_PAC_RET
> +	PACIASP
> +#else
>  	BTI_C
> +#endif
>  	stp	x29, x30, [sp, -16]!
>  	mov	x29, sp
>  
> diff --git a/sysdeps/aarch64/crtn.S b/sysdeps/aarch64/crtn.S
> index 94a6f970ef..e1cb74a572 100644
> --- a/sysdeps/aarch64/crtn.S
> +++ b/sysdeps/aarch64/crtn.S
> @@ -41,10 +41,16 @@
>  
>  	.section .init,"ax",%progbits
>  	ldp	x29, x30, [sp], 16
> +#if ENABLE_PAC_RET
> +	AUTIASP
> +#endif
>  	RET
>  
>  	.section .fini,"ax",%progbits
>  	ldp	x29, x30, [sp], 16
> +#if ENABLE_PAC_RET
> +	AUTIASP
> +#endif
>  	RET
>  
>  END_FILE
> diff --git a/sysdeps/aarch64/dl-tlsdesc.S b/sysdeps/aarch64/dl-tlsdesc.S
> index d55e0443aa..25628d942f 100644
> --- a/sysdeps/aarch64/dl-tlsdesc.S
> +++ b/sysdeps/aarch64/dl-tlsdesc.S
> @@ -183,6 +183,10 @@ _dl_tlsdesc_dynamic:
>  	   callee will trash.  */
>  
>  	/* Save the remaining registers that we must treat as caller save.  */
> +# if ENABLE_PAC_RET
> +	PACIASP
> +	cfi_window_save
> +# endif
>  # define NSAVEXREGPAIRS 8
>  	stp	x29, x30, [sp,#-16*NSAVEXREGPAIRS]!
>  	cfi_adjust_cfa_offset (16*NSAVEXREGPAIRS)
> @@ -233,6 +237,10 @@ _dl_tlsdesc_dynamic:
>  	cfi_adjust_cfa_offset (-16*NSAVEXREGPAIRS)
>  	cfi_restore (x29)
>  	cfi_restore (x30)
> +#if ENABLE_PAC_RET
> +	AUTIASP
> +	cfi_window_save
> +#endif
>  	b	1b
>  	cfi_endproc
>  	.size	_dl_tlsdesc_dynamic, .-_dl_tlsdesc_dynamic
> diff --git a/sysdeps/aarch64/dl-trampoline.S b/sysdeps/aarch64/dl-trampoline.S
> index fba5689d09..c0c4c23128 100644
> --- a/sysdeps/aarch64/dl-trampoline.S
> +++ b/sysdeps/aarch64/dl-trampoline.S
> @@ -127,7 +127,12 @@ _dl_runtime_resolve:
>  	cfi_startproc
>  	.align 2
>  _dl_runtime_profile:
> +# if ENABLE_PAC_RET
> +	PACIASP
> +	cfi_window_save
> +# else
>  	BTI_C
> +# endif
>  	/* AArch64 we get called with:
>  	   ip0		&PLTGOT[2]
>  	   ip1		temp(dl resolver entry point)
> @@ -291,9 +296,17 @@ _dl_runtime_profile:
>  	cfi_def_cfa_register (sp)
>  	ldr	x29, [x29, #0]
>  	cfi_restore(x29)
> +# if ENABLE_PAC_RET
> +	add	sp, sp, SF_SIZE
> +	cfi_adjust_cfa_offset (-SF_SIZE)
> +	AUTIASP
> +	cfi_window_save
> +	add	sp, sp, 16
> +	cfi_adjust_cfa_offset (-16)
> +# else
>  	add	sp, sp, SF_SIZE + 16
>  	cfi_adjust_cfa_offset (- SF_SIZE - 16)
> -
> +# endif
>  	br	lr
>  
>  	cfi_endproc
> diff --git a/sysdeps/aarch64/sysdep.h b/sysdeps/aarch64/sysdep.h
> index 07dc7858a5..63a04a70cd 100644
> --- a/sysdeps/aarch64/sysdep.h
> +++ b/sysdeps/aarch64/sysdep.h
> @@ -45,6 +45,18 @@
>  #define BTI_C		hint	34
>  #define BTI_J		hint	36
>  
> +/* Return address signing support (pac-ret).  */
> +#define ENABLE_PAC_RET 1
> +#if ENABLE_PAC_RET
> +# define PACIASP	hint	25
> +# define AUTIASP	hint	29
> +# define PACIASP_AND_BTI_C	PACIASP
> +#else
> +# define PACIASP
> +# define AUTIASP
> +# define PACIASP_AND_BTI_C	BTI_C
> +#endif
> +

The PACIASP/AUTIASP are already protected by ENABLE_PAC_RET, why do you need
to redefine it for !ENABLE_PAC_RET?

Also, the macro PACIASP_AND_BTI_C is not used in this specific, should it
be defined where it is actually used?

>  #define FEATURE_1_BTI 1
>  #define FEATURE_1_PAC 2
>  
> @@ -61,7 +73,11 @@
>    .word features;			\
>    .word 0;
>  
> -#define END_FILE GNU_PROPERTY(FEATURE_1_BTI)
> +#if ENABLE_PAC_RET
> +# define END_FILE GNU_PROPERTY(FEATURE_1_BTI|FEATURE_1_PAC)
> +#else
> +# define END_FILE GNU_PROPERTY(FEATURE_1_BTI)
> +#endif
>  
>  /* Define an entry point visible from C.  */
>  #define ENTRY(name)						\
> -- 
> 2.17.1