diff mbox series

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

Message ID 20200430174446.GF29015@arm.com
State Superseded
Headers
Series aarch64: branch protection support |

Commit Message

Szabolcs Nagy April 30, 2020, 5:44 p.m. UTC

Comments

Adhemerval Zanella Netto May 8, 2020, 4:59 p.m. UTC | #1 
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
Szabolcs Nagy May 11, 2020, 8:27 a.m. UTC | #2 
The 05/08/2020 13:59, Adhemerval Zanella wrote:
> 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?

the version number is not useful because the fix
can be backported (even to closed branches, just
not upstream).

but gcc 10.1 has no known bug.
(except if -pg and __builtin_return_address
handling changes, i consider current -pg
broken with pac-ret, but we might decide to
fix it up in _mcount instead of the compiler)

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

yeah i changed the configury bits in v2, i didnt
do it in v1 because it's quite painful: gcc
does not set a macro to test for it
Adhemerval Zanella Netto May 11, 2020, 6:39 p.m. UTC | #3 
On 11/05/2020 05:27, Szabolcs Nagy wrote:
> The 05/08/2020 13:59, Adhemerval Zanella wrote:
>> 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?
> 
> the version number is not useful because the fix
> can be backported (even to closed branches, just
> not upstream).
> 
> but gcc 10.1 has no known bug.
> (except if -pg and __builtin_return_address
> handling changes, i consider current -pg
> broken with pac-ret, but we might decide to
> fix it up in _mcount instead of the compiler)

Ack.

> 
>>
>> In any case I think we should add a configure check based on compiler
>> -mbranch-protection= options used to enable/disable ENABLE_PAC_RET.
> 
> yeah i changed the configury bits in v2, i didnt
> do it in v1 because it's quite painful: gcc
> does not set a macro to test for it
> 

Yes, such checks requires to build a code snippet and parse for specific
instructions patterns.
diff mbox series

Patch

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)
---
 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
+
 #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