| Message ID | 20200430174446.GF29015@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | aarch64: branch protection support | expand |
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(-) 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