Skip to content

Add pointer field protection feature. #133538

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: users/pcc/spr/main.add-pointer-field-protection-feature
Choose a base branch
from
Open

Add pointer field protection feature. #133538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bdbe71
[𝘀𝗽𝗿] initial version
pcc Mar 28, 2025
40b2f77
Rebase
pcc May 13, 2025
0d82cb3
Rebase, extract trivially relocatable inhibition condition into a fun…
pcc May 24, 2025
50ab633
Rebase
pcc Jul 9, 2025
5b1e91a
Rebase
pcc Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions clang/include/clang/AST/ASTContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,12 @@ struct TypeInfoChars {
}
};

struct PFPField {
CharUnits structOffset;
CharUnits offset;
FieldDecl *field;
};

/// Holds long-lived AST nodes (such as types and decls) that can be
/// referred to throughout the semantic analysis of a file.
class ASTContext : public RefCountedBase<ASTContext> {
Expand Down Expand Up @@ -3618,6 +3624,22 @@ OPT_LIST(V)

StringRef getCUIDHash() const;

bool isPFPStruct(const RecordDecl *rec) const;
void findPFPFields(QualType Ty, CharUnits Offset,
std::vector<PFPField> &Fields, bool IncludeVBases) const;
bool hasPFPFields(QualType ty) const;
bool isPFPField(const FieldDecl *field) const;

/// Returns whether this record's PFP fields (if any) are trivially
/// relocatable (i.e. may be memcpy'd). This may also return true if the
/// record does not have any PFP fields, so it may be necessary for the caller
/// to check for PFP fields, e.g. by calling hasPFPFields().
bool arePFPFieldsTriviallyRelocatable(const RecordDecl *RD) const;

llvm::SetVector<const FieldDecl *> PFPFieldsWithEvaluatedOffset;
void recordMemberDataPointerEvaluation(const ValueDecl *VD);
void recordOffsetOfEvaluation(const OffsetOfExpr *E);

private:
/// All OMPTraitInfo objects live in this collection, one per
/// `pragma omp [begin] declare variant` directive.
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2460,6 +2460,12 @@ def CountedByOrNull : DeclOrTypeAttr {
let LangOpts = [COnly];
}

def NoPointerFieldProtection : DeclOrTypeAttr {
Copy link
Contributor

Choose a reason for hiding this comment

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

This an ABI break so I don't think it can reasonably an on by default for all structs - we can already see annotations in libc++, and they would be needed on every single struct field.

We can imagine a new platform where this would be the platform ABI, but for every other case it is functionally unusable.

I would recommend attributes to permit opt in and opt out on a per-struct basis, and CLI flags to select the default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are numerous circumstances where the C++ ABI is distinct from the platform ABI and is allowed to change. For example, most Chromium-based web browsers ship with a statically linked libc++, and the same is true for many Android apps and server-side software at Google. In the intended usage model, all of libc++ would be built with a -fexperimental-pointer-field-protection= flag consistent with the rest of the program.

The libc++abi opt-outs that I added are only for opting out pointer fields of RTTI structs that are generated by the compiler. In principle, we could teach the compiler to sign those pointer fields which would let us remove the opt-outs, but there is a lower ROI for subjecting those fields to PFP because the RTTI structs are not typically dynamically allocated.

We may consider adding an attribute to allow opting in in the case where the flag is not passed. But I think we should do that in a followup. We would need to carefully consider how that would interact with the other aspects of the opt-in solution, such as the pointer qualifiers.

let Spellings = [Clang<"no_field_protection">];
let Subjects = SubjectList<[Field], ErrorDiag>;
let Documentation = [Undocumented];
}

def SizedBy : DeclOrTypeAttr {
let Spellings = [Clang<"sized_by">];
let Subjects = SubjectList<[Field], ErrorDiag>;
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/Features.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ FEATURE(shadow_call_stack,
FEATURE(tls, PP.getTargetInfo().isTLSSupported())
FEATURE(underlying_type, LangOpts.CPlusPlus)
FEATURE(experimental_library, LangOpts.ExperimentalLibrary)
FEATURE(pointer_field_protection,
LangOpts.getPointerFieldProtection() !=
LangOptions::PointerFieldProtectionKind::None)

// C11 features supported by other languages as extensions.
EXTENSION(c_alignas, true)
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/LangOptions.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,9 @@ LANGOPT(RelativeCXXABIVTables, 1, 0,
LANGOPT(OmitVTableRTTI, 1, 0,
"Use an ABI-incompatible v-table layout that omits the RTTI component")

ENUM_LANGOPT(PointerFieldProtection, PointerFieldProtectionKind, 2, PointerFieldProtectionKind::None,
"Encode struct pointer fields to protect against UAF vulnerabilities")

LANGOPT(VScaleMin, 32, 0, "Minimum vscale value")
LANGOPT(VScaleMax, 32, 0, "Maximum vscale value")

Expand Down
11 changes: 11 additions & 0 deletions clang/include/clang/Basic/LangOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,17 @@ class LangOptionsBase {
BKey
};

enum class PointerFieldProtectionKind {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I like this being solely a global decision - it makes custom allocators much harder, and it makes it hard for allocators that have different policies, e.g

struct ImportantStruct {
   void *operator new(size_t) { /*tagged allocator*/ }
 };

struct BoringStruct {
   ...
};

struct SomeOtherStruct {
  ImportantStruct *important; // should be tagged pointer
  BoringString *lessImportant; // should be a non tagged pointer
};

This would again require a struct attribute to indicate the pointer policy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that allowing this level of customization should be implemented as part of the separate opt-in solution (e.g. it may be a property of the qualifier). It is generally a reasonable assumption that operator new is the same for all types. In the unlikely case that it isn't, we aren't really doing anything wrong by using the malloc-derived decision here. It just means that we lose some entropy from the type or the pointer tag.

/// Pointer field protection disabled
None,
/// Pointer field protection enabled, allocator does not tag heap
/// allocations.
Untagged,
/// Pointer field protection enabled, allocator is expected to tag heap
/// allocations.
Tagged,
};

enum class ThreadModelKind {
/// POSIX Threads.
POSIX,
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/TokenKinds.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,7 @@ TYPE_TRAIT_2(__is_pointer_interconvertible_base_of, IsPointerInterconvertibleBas

// Clang-only C++ Type Traits
TYPE_TRAIT_1(__is_trivially_relocatable, IsTriviallyRelocatable, KEYCXX)
TYPE_TRAIT_1(__has_non_relocatable_fields, HasNonRelocatableFields, KEYCXX)
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like this -- there are existing functions for handling whether an object is trivially copying, relocatable, etc that should just be updated to correctly report the traits of impacted types. See the various Sema functions querying isAddressDiscriminated(). That function currently only cares about pointer auth, but it would not be unreasonable to have it consider other properties, like PDP.

In fact a lot of the behavior you're wanting in this feature is essentially covered by the PointerAuthQualfier. It would perhaps be reasonable to generalize that to something akin to SecurePointerQualifier that could be either a PAQ or PFP qualifier. Then most of the current semantic queries made to support pointer auth could just be a single shared implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like it either. As mentioned in the main comment, I hope to get rid of this when the P2786 implementation is finalized.

Sharing the qualifiers with PAuth ABI would be a good idea for the separate opt-in solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you misunderstand my intent (I tried to suggest in one the comments or the global comment):

What I'm suggesting is that the qualifier would be implicitly added at the point of declaration, the overarching behavior you're after would be achieved that way. From your other comment about how you determine "C" strictness (just keying off standard layout) that might be challenging as it means you can't really finalize the types of fields until the struct definition is completed, and I don't know if there's anything else in C++ that behaves that way (@cor3ntin might have an idea).

However I'm also not sure how reasonable it is to exclude standard layout types from this protection - what exactly is the rationale for that? (if the reason is a belief that substituting C++ libraries is fine but C libraries is not, I suspect that's not actually very sound, and silently ignoring some structs is similarly a hazard - it can result in "trivial" refactoring invisibly reducing security guarantees).

Regarding P2786: the exact same issues apply to pointer authentication today - trying to solve them separately and outside of the existing triviality queries and operations is not the correct approach.

I'm ok with the logic being distinct from pointer auth, though I would regret that as I believe the shared semantics warrant a shared implementation, but there is no reason for them to be distinct from any of the type queries.

If you want the types to be trivially relocatable - which is not a requirement, it's a performance optimization - then ensure the existing queries and operations handle that correctly. Otherwise you run the risk of other code in clang using the existing trivial behavior queries, and expecting them to be correct, when they are not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you misunderstand my intent (I tried to suggest in one the comments or the global comment):

What I'm suggesting is that the qualifier would be implicitly added at the point of declaration, the overarching behavior you're after would be achieved that way.

No, I understood that part. Please refer to the main comment where I explained why that does not work.

However I'm also not sure how reasonable it is to exclude standard layout types from this protection - what exactly is the rationale for that?

Please refer to the main reply.

silently ignoring some structs is similarly a hazard - it can result in "trivial" refactoring invisibly reducing security guarantees

While it is true that a refactoring has the potential to remove protection from a pointer, the same is also true for adding protection to a pointer (usually code is modified by adding things to objects, and the rules of the language are such that adding something to an object can usually only make it non-standard-layout or non-trivially-copyable). See also "statistical perspective" from the main reply.

Regarding P2786: the exact same issues apply to pointer authentication today - trying to solve them separately and outside of the existing triviality queries and operations is not the correct approach.

The results of the triviality queries are unchanged because PFP does not change the triviality from a language rule perspective. If a type was trivially copyable without PFP, it remains trivially copyable with PFP (i.e. the program is still allowed to memcpy the object, because the pointers are not address discriminated). If a type was trivially relocatable without PFP, it remains trivially relocatable with PFP (i.e. std::trivially_relocate still works because of changes that I will make (not uploaded yet) to __builtin_trivially_relocate in CGBuiltin.cpp). Divergence from the standard C++ triviality rules creates an incompatible dialect of C++, which I think we should avoid.

The copying inhibitions I added in clang/lib/CodeGen are for cases such as those where the object is not trivially copyable according to the language rules but Clang figures out that it can memcpy the object anyway without causing an observable violation of the rules. In other words, Clang is already going outside of the existing triviality queries for optimization purposes, and I am adjusting the conditions under which it does so to account for PFP.

If you want the types to be trivially relocatable - which is not a requirement, it's a performance optimization - then ensure the existing queries and operations handle that correctly. Otherwise you run the risk of other code in clang using the existing trivial behavior queries, and expecting them to be correct, when they are not.

On trivial relocatability, see the main reply.

Code in Clang that uses the standards-defined triviality queries will continue to work. Non-standard-defined queries such as isMemcpyEquivalentSpecialMember and isMemcpyableField need to be adjusted, as previously mentioned. It's possible that someone could add a new non-standard query that breaks PFP (and could also break PAuth ABI because it also uses address discriminated pointers) but that's a bug like any other, which our traditional solution for is to rely on testing to flush out any regressions. The patch series I posted for review has been tested as described in the RFC and the remaining test failures have been assessed to be likely to be bugs in the programs being compiled, so we have a reasonable level of confidence in the implementation.

TYPE_TRAIT_1(__is_trivially_equality_comparable, IsTriviallyEqualityComparable, KEYCXX)
TYPE_TRAIT_1(__is_bounded_array, IsBoundedArray, KEYCXX)
TYPE_TRAIT_1(__is_unbounded_array, IsUnboundedArray, KEYCXX)
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2957,6 +2957,12 @@ defm experimental_omit_vtable_rtti : BoolFOption<"experimental-omit-vtable-rtti"
NegFlag<SetFalse, [], [CC1Option], "Do not omit">,
BothFlags<[], [CC1Option], " the RTTI component from virtual tables">>;

def experimental_pointer_field_protection_EQ : Joined<["-"], "fexperimental-pointer-field-protection=">, Group<f_Group>,
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, not super sure that I like this being a global compile time setting, but maybe it makes sense to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(See other reply.)

Visibility<[ClangOption, CC1Option]>,
Values<"none,untagged,tagged">, NormalizedValuesScope<"LangOptions::PointerFieldProtectionKind">,
NormalizedValues<["None", "Untagged", "Tagged"]>,
MarshallingInfoEnum<LangOpts<"PointerFieldProtection">, "None">;

def fcxx_abi_EQ : Joined<["-"], "fc++-abi=">,
Group<f_clang_Group>, Visibility<[ClangOption, CC1Option]>,
HelpText<"C++ ABI to use. This will override the target C++ ABI.">;
Expand Down
95 changes: 95 additions & 0 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
#include "llvm/Support/Capacity.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/MD5.h"
Expand Down Expand Up @@ -14948,3 +14949,97 @@ bool ASTContext::useAbbreviatedThunkName(GlobalDecl VirtualMethodDecl,
ThunksToBeAbbreviated[VirtualMethodDecl] = std::move(SimplifiedThunkNames);
return Result;
}

bool ASTContext::arePFPFieldsTriviallyRelocatable(const RecordDecl *RD) const {
if (getLangOpts().getPointerFieldProtection() ==
LangOptions::PointerFieldProtectionKind::Tagged)
return !isa<CXXRecordDecl>(RD) ||
cast<CXXRecordDecl>(RD)->hasTrivialDestructor();
return true;
}

bool ASTContext::isPFPStruct(const RecordDecl *rec) const {
if (getLangOpts().getPointerFieldProtection() !=
LangOptions::PointerFieldProtectionKind::None)
if (auto *cxxRec = dyn_cast<CXXRecordDecl>(rec))
return !cxxRec->isStandardLayout();
return false;
}

void ASTContext::findPFPFields(QualType Ty, CharUnits Offset,
std::vector<PFPField> &Fields,
bool IncludeVBases) const {
if (auto *AT = getAsConstantArrayType(Ty)) {
if (auto *ElemDecl = AT->getElementType()->getAsCXXRecordDecl()) {
const ASTRecordLayout &ElemRL = getASTRecordLayout(ElemDecl);
for (unsigned i = 0; i != AT->getSize(); ++i) {
findPFPFields(AT->getElementType(), Offset + i * ElemRL.getSize(),
Fields, true);
}
}
}
auto *Decl = Ty->getAsCXXRecordDecl();
if (!Decl)
return;
const ASTRecordLayout &RL = getASTRecordLayout(Decl);
for (FieldDecl *field : Decl->fields()) {
CharUnits fieldOffset =
Offset + toCharUnitsFromBits(RL.getFieldOffset(field->getFieldIndex()));
if (isPFPField(field))
Fields.push_back({Offset, fieldOffset, field});
findPFPFields(field->getType(), fieldOffset, Fields, true);
}
for (auto &Base : Decl->bases()) {
if (Base.isVirtual())
continue;
CharUnits BaseOffset =
Offset + RL.getBaseClassOffset(Base.getType()->getAsCXXRecordDecl());
findPFPFields(Base.getType(), BaseOffset, Fields, false);
}
if (IncludeVBases) {
for (auto &Base : Decl->vbases()) {
CharUnits BaseOffset =
Offset + RL.getVBaseClassOffset(Base.getType()->getAsCXXRecordDecl());
findPFPFields(Base.getType(), BaseOffset, Fields, false);
}
}
}

bool ASTContext::hasPFPFields(QualType ty) const {
std::vector<PFPField> pfpFields;
findPFPFields(ty, CharUnits::Zero(), pfpFields, true);
return !pfpFields.empty();
}

bool ASTContext::isPFPField(const FieldDecl *field) const {
if (!isPFPStruct(field->getParent()))
return false;
return field->getType()->isPointerType() &&
!field->hasAttr<NoPointerFieldProtectionAttr>();
}

void ASTContext::recordMemberDataPointerEvaluation(const ValueDecl *VD) {
if (getLangOpts().getPointerFieldProtection() ==
LangOptions::PointerFieldProtectionKind::None)
return;
auto *FD = dyn_cast<FieldDecl>(VD);
if (!FD)
FD = cast<FieldDecl>(cast<IndirectFieldDecl>(VD)->chain().back());
if (!isPFPField(FD))
return;
PFPFieldsWithEvaluatedOffset.insert(FD);
}

void ASTContext::recordOffsetOfEvaluation(const OffsetOfExpr *E) {
if (getLangOpts().getPointerFieldProtection() ==
LangOptions::PointerFieldProtectionKind::None ||
E->getNumComponents() == 0)
return;
OffsetOfNode Comp = E->getComponent(E->getNumComponents() - 1);
if (Comp.getKind() != OffsetOfNode::Field)
return;
FieldDecl *FD = Comp.getField();
if (!isPFPField(FD))
return;
PFPFieldsWithEvaluatedOffset.insert(FD);
}
1 change: 1 addition & 0 deletions clang/lib/AST/ExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14932,6 +14932,7 @@ bool IntExprEvaluator::VisitUnaryExprOrTypeTraitExpr(
}

bool IntExprEvaluator::VisitOffsetOfExpr(const OffsetOfExpr *OOE) {
Info.Ctx.recordOffsetOfEvaluation(OOE);
CharUnits Result;
unsigned n = OOE->getNumComponents();
if (n == 0)
Expand Down
4 changes: 3 additions & 1 deletion clang/lib/AST/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2852,7 +2852,9 @@ bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
} else if (!BaseElementType->isObjectType()) {
return false;
} else if (const auto *RD = BaseElementType->getAsRecordDecl()) {
return RD->canPassInRegisters();
return RD->canPassInRegisters() &&
(Context.arePFPFieldsTriviallyRelocatable(RD) ||
!Context.hasPFPFields(BaseElementType));
} else if (BaseElementType.isTriviallyCopyableType(Context)) {
return true;
} else {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/AST/TypePrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2096,6 +2096,9 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::ExtVectorType:
OS << "ext_vector_type";
break;
case attr::NoPointerFieldProtection:
OS << "no_field_protection";
break;
}
OS << "))";
}
Expand Down
Loading
Loading
You are viewing a condensed version of this merge commit. You can view the full changes here.