Skip to content

[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

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

john-brawn-arm
Copy link
Collaborator

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.

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.
@llvmbot
Copy link
Member

llvmbot commented Aug 7, 2024

@llvm/pr-subscribers-libunwind

Author: John Brawn (john-brawn-arm)

Changes

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.

Full diff: https://github.com/llvm/llvm-project/pull/102322.diff

2 Files Affected:

  • (modified) libunwind/src/UnwindRegistersRestore.S (+15-1)
  • (modified) libunwind/src/assembly.h (+20-5)
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
 

Copy link

github-actions bot commented Aug 7, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

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
 

Copy link
Member

@DanielKristofKiss DanielKristofKiss left a 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
Copy link
Member

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>
@john-brawn-arm john-brawn-arm merged commit 3952910 into llvm:main Aug 8, 2024
55 of 56 checks passed
@john-brawn-arm john-brawn-arm deleted the libunwind_gcs_bti branch August 8, 2024 10:26
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 12, 2024
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)
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 20, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants