-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[libunwind] Fix problems caused by combining BTI and GCS #102322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The libunwind assembly files need adjustment in order to work correctly when both BTI and GCS are both enabled (which will be the case when using -mbranch-protection=standard): * __libunwind_Registers_arm64_jumpto can't use br to jump to the return location, instead we need to use gcspush then ret. * Because we indirectly call __libunwind_Registers_arm64_jumpto it needs to start with bit jc. * We need to set the GCS GNU property bit when it's enabled.
@llvm/pr-subscribers-libunwind Author: John Brawn (john-brawn-arm) ChangesThe libunwind assembly files need adjustment in order to work correctly when both BTI and GCS are both enabled (which will be the case when using -mbranch-protection=standard):
Full diff: https://github.com/llvm/llvm-project/pull/102322.diff 2 Files Affected:
diff --git a/libunwind/src/UnwindRegistersRestore.S b/libunwind/src/UnwindRegistersRestore.S
index b407221ca45f8..7783c94088f68 100644
--- a/libunwind/src/UnwindRegistersRestore.S
+++ b/libunwind/src/UnwindRegistersRestore.S
@@ -629,6 +629,10 @@ Lnovec:
#elif defined(__aarch64__)
+#if defined(__ARM_FEATURE_GCS_DEFAULT)
+.arch_extension gcs
+#endif
+
//
// extern "C" void __libunwind_Registers_arm64_jumpto(Registers_arm64 *);
//
@@ -680,7 +684,17 @@ DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto)
ldr x16, [x0, #0x0F8]
ldp x0, x1, [x0, #0x000] // restore x0,x1
mov sp,x16 // restore sp
- br x30 // jump to pc
+#if defined(__ARM_FEATURE_GCS_DEFAULT)
+ // If GCS is enabled we need to push the address we're returning to onto the
+ // GCS stack. We can't just return using br, as there won't be a BTI landing
+ // pad instruction at the destination.
+ mov x16, #1
+ chkfeat x16
+ cbnz x16, Lnogcs
+ gcspushm x30
+Lnogcs:
+#endif
+ ret // jump to pc
#elif defined(__arm__) && !defined(__APPLE__)
diff --git a/libunwind/src/assembly.h b/libunwind/src/assembly.h
index fb07d04071af3..f8e83e138eff5 100644
--- a/libunwind/src/assembly.h
+++ b/libunwind/src/assembly.h
@@ -82,7 +82,22 @@
#define PPC64_OPD2
#endif
-#if defined(__aarch64__) && defined(__ARM_FEATURE_BTI_DEFAULT)
+#if defined(__aarch64__)
+#if defined(__ARM_FEATURE_GCS_DEFAULT) && defined(__ARM_FEATURE_BTI_DEFAULT)
+// Set BTI, PAC, and GCS gnu property bits
+#define GNU_PROPERTY 7
+// We indirectly branch to __libunwind_Registers_arm64_jumpto from
+// __unw_phase2_resume, so we need to use bti jc.
+#define AARCH64_BTI bti jc
+#elif defined(__ARM_FEATURE_GCS_DEFAULT)
+// Set GCS gnu property bit
+#define GNU_PROPERTY 4
+#elif defined(__ARM_FEATURE_BTI_DEFAULT)
+// Set BTI and PAC gnu property bits
+#define GNU_PROPERTY 3
+#define AARCH64_BTI bti c
+#endif
+#ifdef GNU_PROPERTY
.pushsection ".note.gnu.property", "a" SEPARATOR \
.balign 8 SEPARATOR \
.long 4 SEPARATOR \
@@ -91,12 +106,12 @@
.asciz "GNU" SEPARATOR \
.long 0xc0000000 SEPARATOR /* GNU_PROPERTY_AARCH64_FEATURE_1_AND */ \
.long 4 SEPARATOR \
- .long 3 SEPARATOR /* GNU_PROPERTY_AARCH64_FEATURE_1_BTI AND */ \
- /* GNU_PROPERTY_AARCH64_FEATURE_1_PAC */ \
+ .long GNU_PROPERTY SEPARATOR \
.long 0 SEPARATOR \
.popsection SEPARATOR
-#define AARCH64_BTI bti c
-#else
+#endif
+#endif
+#if !defined(AARCH64_BTI)
#define AARCH64_BTI
#endif
|
You can test this locally with the following command:git-clang-format --diff d06303ffc1f3b2023532fd426734e9435f87d038 d64241402d6fc32d34fea649bb65186d259fc8e8 --extensions h -- libunwind/src/assembly.h View the diff from clang-format here.diff --git a/libunwind/src/assembly.h b/libunwind/src/assembly.h
index f8e83e138e..ff004fe887 100644
--- a/libunwind/src/assembly.h
+++ b/libunwind/src/assembly.h
@@ -98,17 +98,12 @@
#define AARCH64_BTI bti c
#endif
#ifdef GNU_PROPERTY
- .pushsection ".note.gnu.property", "a" SEPARATOR \
- .balign 8 SEPARATOR \
- .long 4 SEPARATOR \
- .long 0x10 SEPARATOR \
- .long 0x5 SEPARATOR \
- .asciz "GNU" SEPARATOR \
- .long 0xc0000000 SEPARATOR /* GNU_PROPERTY_AARCH64_FEATURE_1_AND */ \
- .long 4 SEPARATOR \
- .long GNU_PROPERTY SEPARATOR \
- .long 0 SEPARATOR \
- .popsection SEPARATOR
+.pushsection ".note.gnu.property",
+ "a" SEPARATOR.balign 8 SEPARATOR.long 4 SEPARATOR.long 0x10 SEPARATOR
+ .long 0x5 SEPARATOR.asciz "GNU" SEPARATOR
+ .long 0xc0000000 SEPARATOR /* GNU_PROPERTY_AARCH64_FEATURE_1_AND */
+ .long 4 SEPARATOR.long GNU_PROPERTY SEPARATOR.long 0 SEPARATOR
+ .popsection SEPARATOR
#endif
#endif
#if !defined(AARCH64_BTI)
@@ -117,12 +112,13 @@
#if !defined(__aarch64__)
#ifdef __ARM_FEATURE_PAC_DEFAULT
- .eabi_attribute Tag_PAC_extension, 2
- .eabi_attribute Tag_PACRET_use, 1
+ .eabi_attribute Tag_PAC_extension,
+ 2 .eabi_attribute Tag_PACRET_use,
+ 1
#endif
#ifdef __ARM_FEATURE_BTI_DEFAULT
- .eabi_attribute Tag_BTI_extension, 1
- .eabi_attribute Tag_BTI_use, 1
+ .eabi_attribute Tag_BTI_extension,
+ 1 .eabi_attribute Tag_BTI_use, 1
#endif
#endif
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM with a NIT.
Please wait for someone from the unwind group to ack it too.
#define GNU_PROPERTY 7 | ||
// We indirectly branch to __libunwind_Registers_arm64_jumpto from | ||
// __unw_phase2_resume, so we need to use bti jc. | ||
#define AARCH64_BTI bti jc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__unw_getcontext
will also get the bti jc
so slightly weakens the BTI protection but since paciasp
is also bti jc
might not be a big problem.
Co-authored-by: Daniel Kiss <daniel.kristof.kiss@gmail.com>
The libunwind assembly files need adjustment in order to work correctly when both BTI and GCS are both enabled (which will be the case when using -mbranch-protection=standard): * __libunwind_Registers_arm64_jumpto can't use br to jump to the return location, instead we need to use gcspush then ret. * Because we indirectly call __libunwind_Registers_arm64_jumpto it needs to start with bti jc. * We need to set the GCS GNU property bit when it's enabled. --------- Co-authored-by: Daniel Kiss <daniel.kristof.kiss@gmail.com> (cherry picked from commit 3952910)
The libunwind assembly files need adjustment in order to work correctly when both BTI and GCS are both enabled (which will be the case when using -mbranch-protection=standard): * __libunwind_Registers_arm64_jumpto can't use br to jump to the return location, instead we need to use gcspush then ret. * Because we indirectly call __libunwind_Registers_arm64_jumpto it needs to start with bti jc. * We need to set the GCS GNU property bit when it's enabled. --------- Co-authored-by: Daniel Kiss <daniel.kristof.kiss@gmail.com> (cherry picked from commit 3952910)
The libunwind assembly files need adjustment in order to work correctly when both BTI and GCS are both enabled (which will be the case when using -mbranch-protection=standard):