[10/12] aarch64: Add pac-ret support to asm files
Commit Message
Comments
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
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
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.
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(-)
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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