Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
6 changes: 6 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,12 @@ Improvements to Clang's diagnostics
pointers under ``-Wthread-safety-beta`` (still experimental), which reduces
both false positives but also false negatives through more precise analysis.

- A new warning ``-Wptrauth-weak-schema`` has been added to detect variable
declarations of function pointer type and internal linkage that use a weak
pointer authentication schema, either implicitly or by specifying a discriminator
with no diversifiers. This warning applies only to targets supporting pointer
authentication.

Improvements to Clang's time-trace
----------------------------------

Expand Down
9 changes: 5 additions & 4 deletions clang/include/clang/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
return findPointerAuthContent(T) != PointerAuthContent::None;
}

// A simple helper function to short circuit pointer auth checks.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

bool isPointerAuthenticationAvailable() const {
return LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics;
}

private:
llvm::DenseMap<const CXXRecordDecl *, CXXRecordDeclRelocationInfo>
RelocatableClasses;
Expand All @@ -670,10 +675,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
AddressDiscriminatedData
};

// A simple helper function to short circuit pointer auth checks.
bool isPointerAuthenticationAvailable() const {
return LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics;
}
PointerAuthContent findPointerAuthContent(QualType T) const;
mutable llvm::DenseMap<const RecordDecl *, PointerAuthContent>
RecordContainsAddressDiscriminatedPointerAuth;
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,11 @@ def err_ptrauth_address_discrimination_invalid : Error<
def err_ptrauth_extra_discriminator_invalid : Error<
"invalid extra discriminator flag '%0'; '__ptrauth' requires a value between "
"'0' and '%1'">;
def warn_ptrauth_weak_schema
Copy link
Contributor

Choose a reason for hiding this comment

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

@efriedma-quic would it be reasonable to have a note suggesting the use of the __ptrauth qualifier?

: Warning<"%0 has internal linkage with a %select{|default }1"
"pointer authentication schema that should be overridden by "
"%select{a|an explicit}1 schema with unique diversifiers">,
InGroup<DiagGroup<"ptrauth-weak-schema">>;

/// main()
// static main() is not an error in C, just in C++.
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -3976,6 +3976,10 @@ class Sema final : public SemaBase {
void CheckVariableDeclarationType(VarDecl *NewVD);
void CheckCompleteVariableDeclaration(VarDecl *VD);

// Warn about the use of a weak pointer authentication schema on a variable of
// function pointer type and internal linkage.
void DiagnoseWeakPointerAuthenticationSchema(VarDecl *VD);

NamedDecl *ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
TypeSourceInfo *TInfo,
LookupResult &Previous,
Expand Down
12 changes: 12 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7627,6 +7627,16 @@ static bool isMainVar(DeclarationName Name, VarDecl *VD) {
VD->isExternC());
}

void Sema::DiagnoseWeakPointerAuthenticationSchema(VarDecl *VD) {
if (Context.isPointerAuthenticationAvailable() &&
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 I'd prefer separating the pointer auth availability check -- we're planning to add more checks later so I think this should be

if (!Context.isPointerAuthenticationAvailable())
  return;

VD->isFunctionPointerType() && !VD->isExternallyVisible()) {
PointerAuthQualifier Q = VD->getType().getQualifiers().getPointerAuth();
if (!Q || (!Q.isAddressDiscriminated() && Q.getExtraDiscriminator() == 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: single statement if statements don't get braces, even if they are multiple lines. I don't like it, but those are the rules (it would be nice if clang-format could be configured for this)

Diag(VD->getLocation(), diag::warn_ptrauth_weak_schema) << VD << !Q;
}
}
}

NamedDecl *Sema::ActOnVariableDeclarator(
Scope *S, Declarator &D, DeclContext *DC, TypeSourceInfo *TInfo,
LookupResult &Previous, MultiTemplateParamsArg TemplateParamLists,
Expand Down Expand Up @@ -8357,6 +8367,8 @@ NamedDecl *Sema::ActOnVariableDeclarator(
D.isFunctionDefinition());
}

DiagnoseWeakPointerAuthenticationSchema(NewVD);

if (NewTemplate) {
if (NewVD->isInvalidDecl())
NewTemplate->setInvalidDecl();
Expand Down
60 changes: 60 additions & 0 deletions clang/test/Sema/ptrauth-weak-schema.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// RUN: %clang_cc1 -triple arm64e-apple-ios -fptrauth-calls -fptrauth-intrinsics -fsyntax-only -Wno-unused-variable -verify %s
// RUN: %clang_cc1 -triple arm64e-apple-ios -DNO_PTRAUTH -fsyntax-only -Wno-unused-variable -verify=noptrauth %s

// noptrauth-no-diagnostics

#include <ptrauth.h>

#if defined(__PTRAUTH__) == defined(NO_PTRAUTH)
#error expected pointer authentication state does not match actual
#endif

#if defined(NO_PTRAUTH)
#define FN_PTR_AUTH(address_diversity, constant_discriminator)
#else
#define FN_PTR_AUTH(address_diversity, constant_discriminator) \
__ptrauth(ptrauth_key_function_pointer, address_diversity, constant_discriminator)
#endif

// Global variables with external linkage and weak pointer authentication should
// not raise any warning.
extern void(* g1_external_weak)(void);
void(* FN_PTR_AUTH(0, 0) g2_external_weak)(void);

// Global variables with internal linkage and strong pointer authentication
// should not raise any warning.
static void(* FN_PTR_AUTH(1, 65535) g1_internal_strong)(void);
static void(* FN_PTR_AUTH(0, 65535) g2_internal_strong)(void);
static void(* FN_PTR_AUTH(1, 0) g3_internal_strong)(void);

#if !defined(NO_PTRAUTH)
// Global variables with internal linkage and weak pointer authentication should
// raise a warning.
static void(* g1_internal_weak)(void);
// expected-warning@-1 {{'g1_internal_weak' has internal linkage with a default pointer authentication schema that should be overridden by an explicit schema with unique diversifiers}}
static void(* FN_PTR_AUTH(0, 0) g2_internal_weak)(void);
// expected-warning@-1 {{'g2_internal_weak' has internal linkage with a pointer authentication schema that should be overridden by a schema with unique diversifiers}}

// Assert that -Wptrauth-weak-schema silences warnings.
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wptrauth-weak-schema"
static void(* g3_internal_weak)(void);
#pragma clang diagnostic pop
#endif

void test_local_variables(void) {
#if !defined(NO_PTRAUTH)
Copy link
Contributor

Choose a reason for hiding this comment

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

you should remove this ifdef, instead you can make each test use a different prefix. Due to the wildly different amount of tests, I'd just override for the non-ptrauth case, which you can do with -verify=noptrauth and then replace // expected-no-diagnostics with // noptrauth-no-diagnostics

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good

// Local variables (internal linkage) with weak pointer authentication
// should raise a warning.
static void(* l1_internal_weak)(void);
// expected-warning@-1 {{'l1_internal_weak' has internal linkage with a default pointer authentication schema that should be overridden by an explicit schema with unique diversifiers}}
static void(* FN_PTR_AUTH(0, 0) l2_internal_weak)(void);
// expected-warning@-1 {{'l2_internal_weak' has internal linkage with a pointer authentication schema that should be overridden by a schema with unique diversifiers}}
#endif

// Local variables (internal linkage) with strong pointer authentication
// should not raise any warning.
void(* FN_PTR_AUTH(1, 65535) l1_internal_strong)(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

l{1|2}_internal_weak has static storage duration, while l{1|2|3}_internal_strong has scoped storage duration. Is this intended?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @kovdan01 for having a look at this PR. That was an unfortunate coincidence as it should not be relevant. I added the missing combinations to the test to make sure that they work as expected.

void(* FN_PTR_AUTH(0, 65535) l2_internal_strong)(void);
void(* FN_PTR_AUTH(1, 0) l3_internal_strong)(void);
}