[libc++] Annotate classes with _LIBCPP_PFP to enable pointer field protection #151652
Conversation
Created using spr 1.3.6-beta.1
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Peter Collingbourne (pcc) ChangesThe __force_nonstandard_layout class causes types derived from it to be Full diff: https://github.com/llvm/llvm-project/pull/151652.diff 9 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 51444ec668e2b..80e87c9702d7d 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -375,6 +375,7 @@ set(files
__flat_set/flat_set.h
__flat_set/ra_iterator.h
__flat_set/utils.h
+ __force_nonstandard_layout
__format/buffer.h
__format/concepts.h
__format/container_adaptor.h
diff --git a/libcxx/include/__force_nonstandard_layout b/libcxx/include/__force_nonstandard_layout
new file mode 100644
index 0000000000000..6cc748f073b2a
--- /dev/null
+++ b/libcxx/include/__force_nonstandard_layout
@@ -0,0 +1,43 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___FORCE_NONSTANDARD_LAYOUT
+#define _LIBCPP___FORCE_NONSTANDARD_LAYOUT
+
+#include <__config>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// Force a class to be non-standard layout by giving it two bases with the same
+// type. This is useful when structure protection is enabled because structure
+// protection cannot be applied to standard layout classes. We may use this in
+// cases where the standard does not specify whether a standard library class is
+// standard layout. See C++2a [class]p7:
+// A class S is a standard-layout class if it:
+// -- has at most one base class subobject of any given type
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Winaccessible-base")
+class __force_nonstandard_layout_base1 {};
+class __force_nonstandard_layout_base2 : __force_nonstandard_layout_base1 {};
+class __force_nonstandard_layout : __force_nonstandard_layout_base1, __force_nonstandard_layout_base2 {};
+_LIBCPP_DIAGNOSTIC_POP
+
+#if __has_feature(pointer_field_protection)
+# define _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT : __force_nonstandard_layout
+#else
+# define _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT
+#endif
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___FORCE_NONSTANDARD_LAYOUT
diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h
index dc112ebfd0faa..2e2d93662461a 100644
--- a/libcxx/include/__functional/function.h
+++ b/libcxx/include/__functional/function.h
@@ -14,6 +14,7 @@
#include <__config>
#include <__cstddef/nullptr_t.h>
#include <__exception/exception.h>
+#include <__force_nonstandard_layout>
#include <__functional/binary_function.h>
#include <__functional/invoke.h>
#include <__functional/unary_function.h>
@@ -427,7 +428,7 @@ template <class _Fp>
class __policy_func;
template <class _Rp, class... _ArgTypes>
-class __policy_func<_Rp(_ArgTypes...)> {
+class __policy_func<_Rp(_ArgTypes...)> _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
// Inline storage for small objects.
__policy_storage __buf_;
diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 0cbd995105671..e51989874beb4 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -16,6 +16,7 @@
#include <__cstddef/nullptr_t.h>
#include <__cstddef/ptrdiff_t.h>
#include <__exception/exception.h>
+#include <__force_nonstandard_layout>
#include <__functional/binary_function.h>
#include <__functional/operations.h>
#include <__functional/reference_wrapper.h>
@@ -304,7 +305,7 @@ using __shared_ptr_nullptr_deleter_ctor_reqs _LIBCPP_NODEBUG =
#endif
template <class _Tp>
-class _LIBCPP_SHARED_PTR_TRIVIAL_ABI shared_ptr {
+class _LIBCPP_SHARED_PTR_TRIVIAL_ABI shared_ptr _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
struct __nullptr_sfinae_tag {};
public:
@@ -1204,7 +1205,7 @@ inline _LIBCPP_HIDE_FROM_ABI _Dp* get_deleter(const shared_ptr<_Tp>& __p) _NOEXC
#endif // _LIBCPP_HAS_RTTI
template <class _Tp>
-class _LIBCPP_SHARED_PTR_TRIVIAL_ABI weak_ptr {
+class _LIBCPP_SHARED_PTR_TRIVIAL_ABI weak_ptr _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
public:
#if _LIBCPP_STD_VER >= 17
typedef remove_extent_t<_Tp> element_type;
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index eff24546cdc01..e895b23598f2d 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -17,6 +17,7 @@
#include <__config>
#include <__cstddef/nullptr_t.h>
#include <__cstddef/size_t.h>
+#include <__force_nonstandard_layout>
#include <__functional/hash.h>
#include <__functional/operations.h>
#include <__memory/allocator_traits.h> // __pointer
@@ -127,7 +128,7 @@ struct __unique_ptr_deleter_sfinae<_Deleter&> {
#endif
template <class _Tp, class _Dp = default_delete<_Tp> >
-class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI unique_ptr {
+class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI unique_ptr _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
public:
typedef _Tp element_type;
typedef _Dp deleter_type;
@@ -396,7 +397,7 @@ struct __unique_ptr_array_bounds_stored {
};
template <class _Tp, class _Dp>
-class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI unique_ptr<_Tp[], _Dp> {
+class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI unique_ptr<_Tp[], _Dp> _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
public:
typedef _Tp element_type;
typedef _Dp deleter_type;
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index f8bb4f01b1e29..f7aabec524288 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -13,6 +13,7 @@
#include <__algorithm/min.h>
#include <__assert>
#include <__config>
+#include <__force_nonstandard_layout>
#include <__fwd/map.h>
#include <__fwd/pair.h>
#include <__fwd/set.h>
@@ -782,7 +783,7 @@ _LIBCPP_DIAGNOSE_WARNING(!__is_invocable_v<_Compare const&, _Tp const&, _Tp cons
int __diagnose_non_const_comparator();
template <class _Tp, class _Compare, class _Allocator>
-class __tree {
+class __tree _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
public:
using value_type = __get_node_value_type_t<_Tp>;
typedef _Compare value_compare;
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 4307e78f6ddbc..066a32d916b11 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -21,6 +21,7 @@
#include <__assert>
#include <__config>
#include <__debug_utils/sanitizers.h>
+#include <__force_nonstandard_layout>
#include <__format/enable_insertable.h>
#include <__fwd/vector.h>
#include <__iterator/advance.h>
@@ -85,7 +86,7 @@ _LIBCPP_PUSH_MACROS
_LIBCPP_BEGIN_NAMESPACE_STD
template <class _Tp, class _Allocator /* = allocator<_Tp> */>
-class vector {
+class vector _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT {
public:
//
// Types
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 5857a83b5fe14..083f14b43372e 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -2377,6 +2377,9 @@ module std [system] {
module undef_macros {
textual header "__undef_macros"
}
+ module force_nonstandard_layout {
+ header "__force_nonstandard_layout"
+ }
// This module needs to appear after __tree to work around issues with modules in Objective-C++ mode.
module coroutine {
diff --git a/llvm/utils/gn/secondary/libcxx/include/BUILD.gn b/llvm/utils/gn/secondary/libcxx/include/BUILD.gn
index d270686a2a87d..b8cd72e1afbdb 100644
--- a/llvm/utils/gn/secondary/libcxx/include/BUILD.gn
+++ b/llvm/utils/gn/secondary/libcxx/include/BUILD.gn
@@ -1032,6 +1032,7 @@ if (current_toolchain == default_toolchain) {
"__flat_set/flat_set.h",
"__flat_set/ra_iterator.h",
"__flat_set/utils.h",
+ "__force_nonstandard_layout",
"__format/buffer.h",
"__format/concepts.h",
"__format/container_adaptor.h",
|
…ield protection. The __force_nonstandard_layout class causes types derived from it to be non-standard layout. This is beneficial with pointer field protection because, thanks to the standard rules related to standard layout structs, it is likely infeasible to automatically apply PFP to pointer fields of standard layout types. By marking commonly used standard types as non-standard layout, we not only enable PFP on the pointers contained in these types but also types containing a field or base class of one of the standard types, thus allowing many types to automatically receive pointer field protection for their fields. Therefore, make several commonly used standard types derive from __force_nonstandard_layout when PFP is enabled. Pull Request: llvm#151652
libcxx/include/__memory/shared_ptr.h
Outdated
|
|
||
| template <class _Tp> | ||
| class _LIBCPP_SHARED_PTR_TRIVIAL_ABI shared_ptr { | ||
| class _LIBCPP_SHARED_PTR_TRIVIAL_ABI shared_ptr _LIBCPP_MAYBE_FORCE_NONSTANDARD_LAYOUT { |
There was a problem hiding this comment.
I think it's better to add a test file (in the libcxx/test/libcxx subdirectory), verifying static_assert(!std::is_standard_layout<T>::value, ""); for affected types when __has_feature(pointer_field_protection).
ldionne
changed the title
There was a problem hiding this comment.
High level comments and questions:
IIUC, the goal here is for the compiler to be able to apply e.g. pointer authentication on fields of these structs automatically. It can't do so if they are standard layout types, because then users are technically allowed to poke into the binary representation of these types a bit too much. Did I get that right?
Let's take for example std::shared_ptr. Before your patch, it is a standard layout class. However, users technically can't take advantage of that, because the only things that they would be allowed to do is inspect the common initial subsequence of std::shared_ptr, use offsetof with it, or cast a std::shared_ptr<...>* to its first member. None of that applies, because all of its members are private (and reserved names anyway). Hence, making it explicitly non-standard-layout should not be a breaking change for users. Do I follow your line of thinking correctly?
Are there any other affordances provided by standard layout types that I would have missed? Either in terms of API (i.e. what users can do with a SL type), or ABI (e.g. the calling convention changes for SL types like it does for trivial types)? It's important to have a solid grasp of this before we can consider moving forward with this (and decide how to best do it). If this e.g. breaks the ABI in any way, we'll need to have a completely different conversation.
Also, while we're talking about ABI, I assume that in your use case (which I perhaps naively imagine to be applying ptrauth to struct fields), I guess you'd be taking an ABI break to enable that new compiler feature, right?
There was a problem hiding this comment.
I just saw #151651 -- I probably should have looked at that one before this PR, but they appeared in the wrong order in my list.
I do agree with what @philnik777 said in that other review -- I think we need additional context and a RFC to understand what's your underlying plan, what the constraints are and evaluate what is the impact to libc++.
Don't get me wrong, we care about security related proposals so we're likely to want to do something to facilitate what you're doing. But I think we need a high level discussion before we start reviewing individual patches that we (the maintainers) don't fully understand the end destination of.
In the meantime, requesting changes so it shows up as such in the review queue.
That's right. For example, the standard would require that a standard-layout
If the user knows the layout of std::shared_ptr, I think they could access the private fields via the common initial subsequence. In practice it seems highly unlikely that code would do this. So this could technically break the source level API, but that seems unlikely, and in the millions of lines of internal source code that I tested this on, I did not find any practical breakage caused by this.
Theoretically a change to standard-layout-ness for the standard types could break some ABI somewhere (the x86_64 and AArch64 psABIs don't mention standard layout, but maybe some other psABI does, and at least I suppose that some code somewhere could be doing things like making ABI-breaking decisions based on
That's right, and I'd like to take advantage of the ABI break to make some things work better for PFP (like this standard-layout change) when it is enabled. PFP is still experimental, so we may consider making further changes to the libc++ ABI when it is enabled.
Sure. I already have https://discourse.llvm.org/t/rfc-structure-protection-a-family-of-uaf-mitigation-techniques/85555 which covers the feature as a whole (the "On standard layout types" section is most relevant for this PR) but I could try to write a separate RFC that focuses on the libc++ aspects. |
However, even if we make I think a "safer" way (defending against |
|
C++23 has this to say on reinterpret_casts:
and on static_casts it says:
The standard permits pointers to different layout-incompatible types to have different representations:
So as long as |
But IIUC Note that given a I'd like to see |
Reading through the standard again, I think that position may be defensible. A precondition for
So if we can find some way to claim that this is not the case, I think we can get away with it. One possibility is to claim that the pointer field is stored at a non-zero offset, as you suggested. The program is not allowed to add the offset by itself because it doesn't know what it is, and it has no standard way to query it because offsetof is not allowed on non-standard-layout structs (PFP automatically disables itself on fields whose offset is taken with offsetof anyway). Whichever offset the program thinks the pointer is at, we can proceed as if it got it wrong (i.e. access leads to UB). This behavior is somewhat defensible: if the implementation's address space is small enough that pointers are known to always fit into a number of bytes smaller than sizeof(void *), the implementation could choose to store a compressed pointer at the non-zero offset and store something else at offset 0.
I'll add something to the main RFC. This issue is orthogonal to the libc++ changes. |
Created using spr 1.3.6-beta.1
pcc
changed the title
I added this to types which are commonly used, as mentioned in the commit message. I will document that in the coding guidelines. |
Created using spr 1.3.6-beta.1
pcc
changed the title
Done |
libcxx/docs/CodingGuidelines.rst
Outdated
| commonly used vocabulary types with pointer fields are marked with the | ||
| ``_LIBCPP_PFP`` attribute, to give Clang permission to use PFP to protect | ||
| their pointer fields. Newly added vocabulary types should be marked with | ||
| this attribute if they contain pointer fields. |
There was a problem hiding this comment.
If these are the criteria, why isn't string annotated?
There was a problem hiding this comment.
That was an oversight; now added.
Technically, basic_string::__long is the type that needs to be annotated because that's the one with a pointer field, so that's what I did. I also improved the wording here to clarify that internal base classes and fields need the annotations if they have pointer fields.
Created using spr 1.3.6-beta.1
5 participants
The _LIBCPP_PFP macro expands to [[_Clang::pointer_field_protection]]
when PFP is globally enabled. This attribute causes types derived from
it to be considered as PFP types. By marking commonly used standard types
with this attribute, we not only enable PFP on the pointers contained in
these types but also types containing a field or base class of one of
the standard types, thus allowing many types to automatically receive
pointer field protection for their fields. Therefore, mark several
commonly used standard types with _LIBCPP_PFP.