Skip to content
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

[libc++] Upstream ptrauth support in libc++ and libc++abi #84573

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Mar 8, 2024

This is an exact upstreaming of the downstream diff. Minor simplifications can be made in the future but upstreaming as-is will make it easier for us to deal with downstream merge conflicts.

Partially fixes #83805

This is an exact upstreaming of the downstream diff. Minor simplifications
can be made in the future but upstreaming as-is will make it easier for
us to deal with downstream merge conflicts.

Partially fixes llvm#83805
@ldionne ldionne requested review from a team as code owners March 8, 2024 22:04
@ldionne
Copy link
Member Author

ldionne commented Mar 8, 2024

CC @ahmedbougacha @ojhunt

@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc++abi libc++abi C++ Runtime Library. Not libc++. labels Mar 8, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This is an exact upstreaming of the downstream diff. Minor simplifications can be made in the future but upstreaming as-is will make it easier for us to deal with downstream merge conflicts.

Partially fixes #83805


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

3 Files Affected:

  • (modified) libcxx/include/typeinfo (+13-1)
  • (modified) libcxx/src/include/overridable_function.h (+12)
  • (modified) libcxxabi/src/private_typeinfo.cpp (+19)
diff --git a/libcxx/include/typeinfo b/libcxx/include/typeinfo
index dafc7b89248eca..56974a43b361df 100644
--- a/libcxx/include/typeinfo
+++ b/libcxx/include/typeinfo
@@ -276,7 +276,19 @@ struct __type_info_implementations {
           __impl;
 };
 
-class _LIBCPP_EXPORTED_FROM_ABI type_info {
+#    if defined(__arm64__) && __has_cpp_attribute(clang::ptrauth_vtable_pointer)
+#      if __has_feature(ptrauth_type_info_discriminated_vtable_pointer)
+#        define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH                                                                  \
+          [[clang::ptrauth_vtable_pointer(process_independent, address_discrimination, type_discrimination)]]
+#      else
+#        define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH                                                                  \
+          [[clang::ptrauth_vtable_pointer(process_independent, no_address_discrimination, no_extra_discrimination)]]
+#      endif
+#    else
+#      define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH
+#    endif
+
+class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH type_info {
   type_info& operator=(const type_info&);
   type_info(const type_info&);
 
diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h
index 7b0fba10f47d4a..fca66ea6daf7a8 100644
--- a/libcxx/src/include/overridable_function.h
+++ b/libcxx/src/include/overridable_function.h
@@ -13,6 +13,10 @@
 #include <__config>
 #include <cstdint>
 
+#if defined(__arm64e__) && __has_feature(ptrauth_calls)
+#  include <ptrauth.h>
+#endif
+
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
@@ -81,6 +85,14 @@ _LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(_Ret (*__fptr)(_Args...)) no
   uintptr_t __end   = reinterpret_cast<uintptr_t>(&__lcxx_override_end);
   uintptr_t __ptr   = reinterpret_cast<uintptr_t>(__fptr);
 
+#if defined(__arm64e__) && __has_feature(ptrauth_calls)
+  // We must pass a void* to ptrauth_strip since it only accepts a pointer type. Also, in particular,
+  // we must NOT pass a function pointer, otherwise we will strip the function pointer, and then attempt
+  // to authenticate and re-sign it when casting it to a uintptr_t again, which will fail because we just
+  // stripped the function pointer. See rdar://122927845.
+  __ptr = reinterpret_cast<uintptr_t>(ptrauth_strip(reinterpret_cast<void*>(__ptr), ptrauth_key_function_pointer));
+#endif
+
   // Finally, the function was overridden if it falls outside of the section's bounds.
   return __ptr < __start || __ptr > __end;
 }
diff --git a/libcxxabi/src/private_typeinfo.cpp b/libcxxabi/src/private_typeinfo.cpp
index 5c68f3e994cd9e..9e58501a559342 100644
--- a/libcxxabi/src/private_typeinfo.cpp
+++ b/libcxxabi/src/private_typeinfo.cpp
@@ -51,6 +51,21 @@
 #include <atomic>
 #endif
 
+#if __has_feature(ptrauth_calls)
+#include <ptrauth.h>
+#endif
+
+
+template<typename T>
+static inline
+T *
+get_vtable(T *vtable) {
+#if __has_feature(ptrauth_calls)
+    vtable = ptrauth_strip(vtable, ptrauth_key_cxx_vtable_pointer);
+#endif
+    return vtable;
+}
+
 static inline
 bool
 is_equal(const std::type_info* x, const std::type_info* y, bool use_strcmp)
@@ -103,6 +118,7 @@ void dyn_cast_get_derived_info(derived_object_info* info, const void* static_ptr
     info->dynamic_type = *(reinterpret_cast<const __class_type_info* const*>(ptr_to_ti_proxy));
 #else
     void **vtable = *static_cast<void ** const *>(static_ptr);
+    vtable = get_vtable(vtable);
     info->offset_to_derived = reinterpret_cast<ptrdiff_t>(vtable[-2]);
     info->dynamic_ptr = static_cast<const char*>(static_ptr) + info->offset_to_derived;
     info->dynamic_type = static_cast<const __class_type_info*>(vtable[-1]);
@@ -561,6 +577,7 @@ __base_class_type_info::has_unambiguous_public_base(__dynamic_cast_info* info,
     offset_to_base = __offset_flags >> __offset_shift;
     if (is_virtual) {
       const char* vtable = *static_cast<const char* const*>(adjustedPtr);
+      vtable = get_vtable(vtable);
       offset_to_base = update_offset_to_base(vtable, offset_to_base);
     }
   } else if (!is_virtual) {
@@ -1501,6 +1518,7 @@ __base_class_type_info::search_above_dst(__dynamic_cast_info* info,
     if (__offset_flags & __virtual_mask)
     {
         const char* vtable = *static_cast<const char*const*>(current_ptr);
+        vtable = get_vtable(vtable);
         offset_to_base = update_offset_to_base(vtable, offset_to_base);
     }
     __base_type->search_above_dst(info, dst_ptr,
@@ -1521,6 +1539,7 @@ __base_class_type_info::search_below_dst(__dynamic_cast_info* info,
     if (__offset_flags & __virtual_mask)
     {
         const char* vtable = *static_cast<const char*const*>(current_ptr);
+        vtable = get_vtable(vtable);
         offset_to_base = update_offset_to_base(vtable, offset_to_base);
     }
     __base_type->search_below_dst(info,

Copy link

github-actions bot commented Mar 8, 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 1cf428a05a62711200d37a1b12722ca16c33a6ea 014c574f325336c78a5ad0dab2665c393726b6df -- libcxx/include/typeinfo libcxx/src/include/overridable_function.h libcxxabi/src/private_typeinfo.cpp
View the diff from clang-format here.
diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h
index fca66ea6da..f608e698ee 100644
--- a/libcxx/src/include/overridable_function.h
+++ b/libcxx/src/include/overridable_function.h
@@ -85,13 +85,13 @@ _LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(_Ret (*__fptr)(_Args...)) no
   uintptr_t __end   = reinterpret_cast<uintptr_t>(&__lcxx_override_end);
   uintptr_t __ptr   = reinterpret_cast<uintptr_t>(__fptr);
 
-#if defined(__arm64e__) && __has_feature(ptrauth_calls)
+#  if defined(__arm64e__) && __has_feature(ptrauth_calls)
   // We must pass a void* to ptrauth_strip since it only accepts a pointer type. Also, in particular,
   // we must NOT pass a function pointer, otherwise we will strip the function pointer, and then attempt
   // to authenticate and re-sign it when casting it to a uintptr_t again, which will fail because we just
   // stripped the function pointer. See rdar://122927845.
   __ptr = reinterpret_cast<uintptr_t>(ptrauth_strip(reinterpret_cast<void*>(__ptr), ptrauth_key_function_pointer));
-#endif
+#  endif
 
   // Finally, the function was overridden if it falls outside of the section's bounds.
   return __ptr < __start || __ptr > __end;
diff --git a/libcxxabi/src/private_typeinfo.cpp b/libcxxabi/src/private_typeinfo.cpp
index 9e58501a55..f9d9c56a6d 100644
--- a/libcxxabi/src/private_typeinfo.cpp
+++ b/libcxxabi/src/private_typeinfo.cpp
@@ -52,18 +52,15 @@
 #endif
 
 #if __has_feature(ptrauth_calls)
-#include <ptrauth.h>
+#  include <ptrauth.h>
 #endif
 
-
-template<typename T>
-static inline
-T *
-get_vtable(T *vtable) {
+template <typename T>
+static inline T* get_vtable(T* vtable) {
 #if __has_feature(ptrauth_calls)
-    vtable = ptrauth_strip(vtable, ptrauth_key_cxx_vtable_pointer);
+  vtable = ptrauth_strip(vtable, ptrauth_key_cxx_vtable_pointer);
 #endif
-    return vtable;
+  return vtable;
 }
 
 static inline

Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after addressing or resolving inline comments.

libcxx/src/include/overridable_function.h Show resolved Hide resolved
@@ -276,7 +276,19 @@ struct __type_info_implementations {
__impl;
};

class _LIBCPP_EXPORTED_FROM_ABI type_info {
# if defined(__arm64__) && __has_cpp_attribute(clang::ptrauth_vtable_pointer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like configuration that might be better suited for a config header?

type_info is weird magic though, so I understand if you disagree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, to support ELF platforms that don't define __arm64__ we ended up rewriting the feature check as:

+#if __has_feature(ptrauth_calls) &&                          \
+    (__has_feature(ptrauth_vtable_address_discrimination) || \
+     __has_feature(ptrauth_vtable_type_discrimination)) &&   \
+    __has_cpp_attribute(clang::ptrauth_vtable_pointer)

static inline
T *
get_vtable(T *vtable) {
#if __has_feature(ptrauth_calls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please give this a more descriptive name than get_vtable, because we already have the vtable in most cases when we call this so it's a touch confusing.

Maybe a name that ties it to ptrauth?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, it may be possible for __builtin_get_vtable() to be used as instead, but I'm not sure what the considerations were when this code was written (or if it predates that builtin?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic_cast machinery had to strip but __builtin_get_vtable always authenticates, so as-is this probably needs to be a manual ptrauth_strip yeah

@ldionne
Copy link
Member Author

ldionne commented Mar 8, 2024

For context, the way I will apply these comments is by making another commit that makes any changes we want and committing them separately. That way, the first commit can go as-is in the tree to reduce merge conflicts downstream.

@ojhunt
Copy link
Contributor

ojhunt commented Apr 3, 2024

heads up @devincoughlin

libcxxabi/src/private_typeinfo.cpp Show resolved Hide resolved
libcxxabi/src/private_typeinfo.cpp Show resolved Hide resolved
@ldionne ldionne merged commit 98244c4 into llvm:main Apr 3, 2024
52 of 54 checks passed
@ldionne ldionne deleted the review/upstream-ptrauth-libcxx branch April 3, 2024 12:04
ldionne added a commit to ldionne/llvm-project that referenced this pull request Apr 3, 2024
This patch applies the comments provided on llvm#84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already
had ptrauth support.
@ldionne
Copy link
Member Author

ldionne commented Apr 3, 2024

I am applying comments in #87481.

kovdan01 pushed a commit to kovdan01/llvm-project that referenced this pull request May 23, 2024
This is an exact upstreaming of the downstream diff. Minor
simplifications can be made in the future but upstreaming as-is will
make it easier for us to deal with downstream merge conflicts.

Partially fixes llvm#83805
ldionne added a commit to ldionne/llvm-project that referenced this pull request Jun 7, 2024
This patch applies the comments provided on llvm#84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already
had ptrauth support.
ldionne added a commit to ldionne/llvm-project that referenced this pull request Jul 18, 2024
This patch applies the comments provided on llvm#84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already
had ptrauth support.
ldionne added a commit to ldionne/llvm-project that referenced this pull request Jul 23, 2024
This patch applies the comments provided on llvm#84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already
had ptrauth support.
ldionne added a commit that referenced this pull request Jul 23, 2024
…#87481)

This patch applies the comments provided on #84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already had
ptrauth support.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 23, 2024
…llvm#87481)

This patch applies the comments provided on llvm#84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already had
ptrauth support.

(cherry picked from commit e64e745)
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…#87481)

Summary:
This patch applies the comments provided on #84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already had
ptrauth support.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251081
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 29, 2024
…llvm#87481)

This patch applies the comments provided on llvm#84573. This is done as a
separate PR to avoid merge conflicts with downstreams that already had
ptrauth support.

(cherry picked from commit e64e745)
asl added a commit that referenced this pull request Sep 9, 2024
…7498)

Apparently, there are two almost identical implementations: one for
MachO and another one for ELF. The ELF bits somehow slipped while
#84573 was reviewed.

The particular implementation is identical to MachO case.
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
…m#107498)

Apparently, there are two almost identical implementations: one for
MachO and another one for ELF. The ELF bits somehow slipped while
llvm#84573 was reviewed.

The particular implementation is identical to MachO case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++. libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[PAC] Make libcxx / libcxxabi pauth-friendly
6 participants