[clang][ptrauth] Warn about the use of a weak signing schema #157779

martinuy · 2025-09-10T01:57:03Z

In this change we added the -Wptrauth-weak-schema diagnostic (enabled by default on targets that support pointer authentication) to warn about the use of a weak signing schema for function pointers stored in global variables with internal linkage.

rdar://159299739

github-actions · 2025-09-10T01:57:23Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

martinuy · 2025-09-10T01:59:50Z

@ojhunt May I ask you to have a look at this draft PR? Thanks!

ojhunt

Requesting a few changes :D

ojhunt · 2025-09-10T03:47:41Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

Possibly something like "%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" (cc @cor3ntin as he is better at wording than I am).

Rather than an actually declared group, let's just go for an inline definition of the warning group for now

InGroup<DiagGroup<"ptrauth-weak-schema">>

Sounds good

ojhunt · 2025-09-10T04:07:07Z

clang/lib/Sema/SemaDecl.cpp

Not for this PR, but we may want to warn on no address discrimination or a zero discriminator. Both scenarios are attackable.

Rather than directly querying getLangOpts().PointerAuthCalls use Context.isPointerAuthenticationAvailable() which covers both intrinsics and call -- arguably the Intrinsics one is the only option we would really need to consider here, but we're trying to move to the general Context query over time.

isExternallyVisible(NewVD->getLinkageInternal())

This should be possible is NewVD->isExternallyVisible()

There's a helper function I have been meaning to upstream though it currently would not be directly used which handles implicit schemas automatically, and that might make this easier to manage.

Sounds good. I just want to point out that ASTContext::isPointerAuthenticationAvailable has private visibility. I will assume that you are thinking of making this method public.

ojhunt · 2025-09-10T04:11:50Z

clang/test/Sema/ptrauth-weak-schema.c

Rather than this check I think you can get away with

#if defined(__PTRAUTH__) == defined(NO_PTRAUTH) #error pointer authentication enabled for non-ptrauth test #endif

Sounds good. Your check seems to work for both cases (i.e. ptrauth test with pointer authentication disabled and non-ptrauth test with pointer authentication enabled), but the message gives me the impression that you were thinking of the latter. I was originally thinking of the former, but I am not sure of how much value it adds anyways. I'll propose to remove it for now -in the interest of not having redundant checks- but let me know if you want to have it.

Oh, it should be "expected pointer authentication state does not match actual"

Sounds good. I'll add the check with the suggested message.

clang/test/Sema/ptrauth-weak-schema.c

ojhunt · 2025-09-10T04:16:38Z

clang/test/Sema/ptrauth-weak-schema.c

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

Sounds good

ojhunt · 2025-09-10T04:17:01Z

clang/test/Sema/ptrauth-weak-schema.c

just put this in the command line :D

Makes sense, there is no reason for such granularity :)

llvmbot · 2025-09-11T00:12:30Z

@llvm/pr-subscribers-clang

Author: Martin Balao Alonso (martinuy)

Changes

In this change we added the -Wptrauth-weak-schema diagnostic (enabled by default on targets that support pointer authentication) to warn about the use of a weak signing schema for function pointers stored in global variables with internal linkage.

rdar://159299739

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

4 Files Affected:

(modified) clang/include/clang/AST/ASTContext.h (+5-4)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5)
(modified) clang/lib/Sema/SemaDecl.cpp (+11)
(added) clang/test/Sema/ptrauth-weak-schema.c (+56)

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 1c17333b722f8..1580968e857d7 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -659,6 +659,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
     return findPointerAuthContent(T) != PointerAuthContent::None;
   }
 
+  // A simple helper function to short circuit pointer auth checks.
+  bool isPointerAuthenticationAvailable() const {
+    return LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics;
+  }
+
 private:
   llvm::DenseMap<const CXXRecordDecl *, CXXRecordDeclRelocationInfo>
       RelocatableClasses;
@@ -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;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index ecdbeb0687cac..f1356fef1961f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -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
+    : 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++.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 7c1459e320167..7a7abf1c4b627 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -8357,6 +8357,17 @@ NamedDecl *Sema::ActOnVariableDeclarator(
                                    D.isFunctionDefinition());
   }
 
+  // Warn about the use of a weak pointer authentication schema on a variable
+  // with internal linkage.
+  if (Context.isPointerAuthenticationAvailable() &&
+      NewVD->isFunctionPointerType() && !NewVD->isExternallyVisible()) {
+    PointerAuthQualifier Q = NewVD->getType().getQualifiers().getPointerAuth();
+    if (!Q || (!Q.isAddressDiscriminated() && Q.getExtraDiscriminator() == 0)) {
+      Diag(NewVD->getLocation(), diag::warn_ptrauth_weak_schema)
+          << NewVD << (Q ? 0 : 1);
+    }
+  }
+
   if (NewTemplate) {
     if (NewVD->isInvalidDecl())
       NewTemplate->setInvalidDecl();
diff --git a/clang/test/Sema/ptrauth-weak-schema.c b/clang/test/Sema/ptrauth-weak-schema.c
new file mode 100644
index 0000000000000..e0aa89efc47ab
--- /dev/null
+++ b/clang/test/Sema/ptrauth-weak-schema.c
@@ -0,0 +1,56 @@
+// 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(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)
+    // 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);
+    void(* FN_PTR_AUTH(0, 65535) l2_internal_strong)(void);
+    void(* FN_PTR_AUTH(1, 0) l3_internal_strong)(void);
+}

ojhunt · 2025-09-11T01:27:03Z

clang/include/clang/AST/ASTContext.h

ojhunt · 2025-09-11T01:28:27Z

clang/lib/Sema/SemaDecl.cpp

I think !Q is more consistent with what other diagnostics do

Sounds good. I will replace the conditional expression with (!Q ? 1 : 0).

I mean just !Q as a boolean is just 0 or 1 from the point of view of the diagnostics.

ojhunt · 2025-09-11T02:08:30Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

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

clang/test/Sema/ptrauth-weak-schema.c

In this change we added the -Wptrauth-weak-schema diagnostic (enabled by default on targets that support pointer authentication) to warn about the use of a weak signing schema for function pointers stored in global variables with internal linkage. rdar://159299739

@ojhunt

Changes after @ojhunt's review on 2025-09-10 (llvm#157779)

ojhunt

Needs update to clang/docs/ReleaseNotes.rst

ojhunt · 2025-09-11T20:37:00Z

clang/lib/Sema/SemaDecl.cpp

                                   D.isFunctionDefinition());
  }

+  // Warn about the use of a weak pointer authentication schema on a variable


This should be pulled into a separate function - we'll want to use this for other declaration types over time, and it will likely become more complex as additional rules occur. Something like

bool Sema::DiagnoseWeakPointerAuthenticationSchema(NamedDecl *ND) { ... }

It's possible we'll need other information over time, but I think for now that's sufficient.

Sounds good, but I have a question. My understanding is that you are thinking of moving both the condition we check and the Diag call into a separate function. Considering that no action such as invalidating the declaration would be taken on the caller side -at least for now-, shouldn't the return type of the separate function be void (similar to void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *) or void Sema::DiagnoseAutoDeductionFailure(const VarDecl *, const Expr *)?

Yeah - I think that's an easier interface to use, otherwise all the code ends up having to do:

if (ShouldDiagnoseBlah(...)) DiagnoseBlah(...); // If you construct the diagnostic inline then as the checks become // richer every call site has to work out the full diagnostic message

And yes, the result should be void - I was thinking in terms of Sema checks that can fail the diagnostics due to the error breaking further tests which is not the case here: warnings by definition don't cause this problem.

One more thing related to the separate function. The class NamedDecl does not contain the DeclType member which we use for the check. I will propose for the parameter type to be ValueDecl but shouldn't we go straight with VarDecl instead? I could not find a case in which the function will receive an EnumConstantDecl (the other subclass of ValueDecl ), and the only caller passes an argument of (static) VarDecl type.

Ok, let's start with vardecl for now, we'll probably need to change to nameddecl in future and add many casts which will be sad - but we can cross that bridge when we get there.

ojhunt

Moar changes sorry! (they're minor style changes)

ojhunt · 2025-09-12T00:19:14Z

clang/lib/Sema/SemaDecl.cpp

 }

+void Sema::DiagnoseWeakPointerAuthenticationSchema(VarDecl *VD) {
+  if (Context.isPointerAuthenticationAvailable() &&


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;

ojhunt · 2025-09-12T00:20:19Z

clang/lib/Sema/SemaDecl.cpp

+  if (Context.isPointerAuthenticationAvailable() &&
+      VD->isFunctionPointerType() && !VD->isExternallyVisible()) {
+    PointerAuthQualifier Q = VD->getType().getQualifiers().getPointerAuth();
+    if (!Q || (!Q.isAddressDiscriminated() && Q.getExtraDiscriminator() == 0)) {


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)

kbeyls · 2025-09-12T08:05:14Z

Thanks for this contribution @martinuy !

My first thought when seeing this warning is that potentially many developers who happen to be targeting a platform with pointer authentication may not understand well enough why a particular signing schema is weak and how they could or should change it.

Side thought: Would this warning only trigger on variables where a developer has explicitly added something to the source code to request a non-default signing schema? (In that case, chances are higher that the developer may be able to understand the warning well).

Basically, I'm wondering if there already is clang documentation that explains why a particular signing schema may be weak. If not, I'm wondering if it could be added somewhere. The most obvious place might be to somehow add it somewhere to https://clang.llvm.org/docs/PointerAuthentication.html?
I'm assuming that a developer who does not understand the warning well may do a web search with the produced warning, and ideally they should find a reasonable explanation when doing so.

ojhunt · 2025-09-12T08:28:52Z

Side thought: Would this warning only trigger on variables where a developer has explicitly added something to the source code to request a non-default signing schema? (In that case, chances are higher that the developer may be able to understand the warning well).

This one I can answer: there's explicit authentication (explicit __ptrauth). I think in this case it's reasonable to assume enough awareness to understand the reason for the warning.

The more tricky one is something like function pointers: they're implicitly signed so a dev only sees

static void(*f)();

Or whatever the cursed syntax is :D

In this case there's no existing qualifier, so no real reason to expect there to be any dev awareness. That's why I was thinking that maybe an additional note might be appropriate? I didn't think an explanation should be in the warning itself?

We're currently hoping that overriding this will be rare enough that suppressing the warning is uncommon.

Eventually we may extend this warning to cases where overriding is more likely to be necessary, but I have an idea of how that can be addressed (I just need to make sure it would actually work :D )

martinuy · 2025-09-12T16:35:50Z

Thanks @kbeyls for having a look at this proposal. You raised a valid concern: a developer that is not explicitly adding the __ptrauth qualifier to their source code and is targeting an architecture that supports pointer authentication might be caught by surprise with this new warning.

My initial impression was that the Pointer Authentication doc offers some clues. In particular, section C function pointers says On arm64e, C function pointers are currently signed with the IA key without address diversity and with a constant discriminator of 0. (...) The use of a uniform constant discriminator greatly simplifies the adoption of arm64e, but it is a significant weakness in the mitigation because it allows any C function pointer to be replaced with another. @ojhunt what do you think of adding a reference to the new warning in that section so we make it easier for developers to connect the dots (e.g. when they search for the warning directly in the document or though a search engine)?

kovdan01 · 2025-09-15T09:49:13Z

clang/test/Sema/ptrauth-weak-schema.c

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


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

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.

atrosinenko · 2025-09-18T14:01:17Z

A side thought: what is the expected effect of this warning (namely, treating implicit signing the same way as __ptrauth qualifiers) when building an existing code base for a new target that has function pointer signing enabled? My initial assumption is that in a ptrauth-unaware C code most function pointer uses would be diagnosed. I'm not sure what the effect should be, but I guess if my assumption is correct this could be quite unintended.

I mean something like this imaginary example:

#include <stdlib.h>

typedef int (*cmp_t)(const void *, const void *);

void my_sort(void *base, size_t nmemb, cmp_t comparator);

void do_sort(void *base, size_t nmemb, size_t size, cmp_t comparator) {
  cmp_t cmp = comparator; // <- warning
  if (size == 8)
    my_sort(base, nmemb, cmp);
  else
    qsort(base, nmemb, size, cmp);
}

martinuy · 2025-10-10T22:24:53Z

A side thought: what is the expected effect of this warning (namely, treating implicit signing the same way as __ptrauth qualifiers) when building an existing code base for a new target that has function pointer signing enabled? My initial assumption is that in a ptrauth-unaware C code most function pointer uses would be diagnosed. I'm not sure what the effect should be, but I guess if my assumption is correct this could be quite unintended.

It's a fair point, thanks for bringing it up. My thinking is that building a pointer authentication unaware code base for a new target that supports the feature shouldn't be enough reason for this warning to pop up. I would expect this warning only if a compilation argument that turns pointer authentication on is explicitly passed, such as -fptrauth-calls. If pointer authentication is intended to be on and the argument is, thus, passed, the warning would be a good opportunity to make a conscious decision: turn pointer authentication off, harden the code base, turn the warning off, etc.

Pointer Authentication documentation.

martinuy · 2025-10-10T23:19:04Z

I pushed a couple of doc changes based on the last discussions we had in this PR.

Please let me know if you have any other concern, request or if it looks good to you.

ojhunt requested changes Sep 10, 2025

View reviewed changes

martinuy marked this pull request as ready for review September 10, 2025 23:21

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 10, 2025

ojhunt requested changes Sep 11, 2025

View reviewed changes

martinuy added 3 commits September 11, 2025 09:37

Changes after @ojhunt's review on 2025-09-09 (llvm#157779)

bc4d0a7

fixup! Changes after @ojhunt's review on 2025-09-09 (llvm#157779)

1d90050

Changes after @ojhunt's review on 2025-09-10 (llvm#157779)

martinuy force-pushed the martinuy/ptrauth-weak-schema-diagnostics branch from 5e3f44c to 1d90050 Compare September 11, 2025 16:38

asl requested review from atrosinenko and kovdan01 September 11, 2025 20:33

ojhunt requested changes Sep 11, 2025

View reviewed changes

ojhunt assigned martinuy Sep 11, 2025

Changes after @ojhunt's review on 2025-09-11 (llvm#157779)

5be722d

ojhunt requested changes Sep 12, 2025

View reviewed changes

Changes after @ojhunt's review on 2025-09-11 - v2 (llvm#157779)

5ff3d60

kovdan01 reviewed Sep 15, 2025

View reviewed changes

Changes after @kovdan01's review on 2025-09-15 (llvm#157779)

b623b0e

Reference to the -Wptrauth-weak-schema diagnostic added to the

7abb90c

Pointer Authentication documentation.

[clang][ptrauth] Warn about the use of a weak signing schema #157779

Are you sure you want to change the base?

[clang][ptrauth] Warn about the use of a weak signing schema #157779

Uh oh!

Conversation

martinuy commented Sep 10, 2025

Uh oh!

github-actions bot commented Sep 10, 2025

Uh oh!

martinuy commented Sep 10, 2025

Uh oh!

ojhunt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Sep 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ojhunt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ojhunt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbeyls commented Sep 12, 2025

Uh oh!

ojhunt commented Sep 12, 2025

Uh oh!

martinuy commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

martinuy commented Sep 12, 2025 •

edited

Loading