[Clang][Sema] Add -Wincompatible-function-pointer-types-strict

[samitolvanen]

Summary

Clang supports indirect call Control-Flow Integrity (CFI) sanitizers
(e.g. -fsanitize=cfi-icall), which enforce an exact type match
between a function pointer and the target function. Unfortunately,
Clang doesn't provide diagnostics that help developers avoid
function pointer assignments that can lead to runtime CFI
failures. -Wincompatible-function-pointer-types doesn't warn about
enum to integer mismatches if the types are otherwise compatible, for
example, which isn't sufficient with CFI.

Add -Wincompatible-function-pointer-types-strict, which checks for a
stricter function type compatibility in assignments and helps warn about
assignments that can potentially lead to CFI failures.

[samitolvanen]

Similarly to D134831, -Wincompatible-function-pointer-types also seems to miss int <-> enum mismatches in function pointer assignments, for example. Does this look like a reasonable approach or is there a better way to implement something like this?

[aaron.ballman]

Thank you for the new diagnostic, please be sure to add a release note so users know there's a new warning.

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

We don't typically add new off-by-default warnings because we have evidence that users don't enable them enough to be worth adding them. Is there a way we can enable this warning by default for CFI compilation units (or when the cfi sanitizer is enabled) so that it's only off by default for non-CFI users? I don't think we have any examples of doing this in the code base, so I believe this would be breaking new ground (and thus is worth thinking about more, perhaps it's a bad idea for some reason).

[samitolvanen]

In D136790#3897184, @aaron.ballman wrote:

please be sure to add a release note so users know there's a new warning.

I'll add a release note. Thanks for the reminder!

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

Is there a way we can enable this warning by default for CFI compilation units (or when the cfi sanitizer is enabled) so that it's only off by default for non-CFI users?

I can look into it, but I'm not sure if there's a convenient way to do that. An alternative to this could be to enable the warning by default, but only actually perform the check if CFI is enabled. This way non-CFI users would never see the warning because this really isn't a concern for them, but CFI users would still get the warning without explicitly enabling it. Or do you think this behavior would be confusing?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

Heh, sorry for not being clear, that's actually somewhat along the lines of how I envisioned you'd implement it (we don't have a way in tablegen to tie DefaultIgnore to some language options or a target).

I was thinking the diagnostic would be left as DefaultIgnore in the .td file, but we'd take note in the driver that CFI was enabled and pass -Wincompatible-function-pointer-types-strict to the -cc1 invocation. If the user wrote -Wno-incompatible-function-pointer-types-strict at the driver level, that would come *after* the automatically generated flag for cc1 and would still disable the diagnostic in the cc1 invocation (because the last flag wins). WDYT?

[nathanchance]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

I do not think that is unreasonable behavior in the generic case but specifically for the Linux kernel, kCFI is disabled by default (CONFIG_CFI_CLANG has to be specifically enabled) but we want to see this warning regardless of the value of that configuration so that driver authors who test with clang and automated testers are more likely to find them. As a result, if the warning is not going to be default on, we would still need to specifically enable it in our CFLAGS. I suppose your suggestion would help out folks using CFI in production though so maybe it is still worth considering doing?

[samitolvanen]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

I was thinking the diagnostic would be left as DefaultIgnore in the .td file, but we'd take note in the driver that CFI was enabled and pass -Wincompatible-function-pointer-types-strict to the -cc1 invocation.

Thanks for clarifying, that sounds reasonable. Of course, enabling it by default with CFI would still mean that we'll have to either explicitly disable this in the kernel (or fix all the existing warnings) before submitting the Clang change.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

As a result, if the warning is not going to be default on, we would still need to specifically enable it in our CFLAGS. I suppose your suggestion would help out folks using CFI in production though so maybe it is still worth considering doing?

It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).

8219

Thanks for clarifying, that sounds reasonable. Of course, enabling it by default with CFI would still mean that we'll have to either explicitly disable this in the kernel (or fix all the existing warnings) before submitting the Clang change.

Er, is there a chicken and egg problem here? I was imagining that you could land the changes in Clang and disable the flag in the Linux kernel around roughly the same time (we can time when we land things to make life easier for you) so it wouldn't be particularly disruptive.

If it would be disruptive, another approach would be to leave it off by default everywhere, get the Linux kernel CFI warning free, then enable it for CFI builds by default. But that seems riskier to me (we are more likely to forget to come turn the warning on by default later).

[nathanchance]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).

Right, I was only mentioning our use case will necessitate it being always enabled so conditionally enabling it would be extra work that wouldn't benefit us but obviously, the world does not revolve around our one project :)

8219

Er, is there a chicken and egg problem here? I was imagining that you could land the changes in Clang and disable the flag in the Linux kernel around roughly the same time (we can time when we land things to make life easier for you) so it wouldn't be particularly disruptive.

Yes, we do not have as much freedom with pushing changes into Linux mainline, as they first have to soak in the -next tree, so that will require at least a week. I assume @kees can help us out with getting that patch to Linus in a timely manner though so that is probably not as much of a concern.

If it would be disruptive, another approach would be to leave it off by default everywhere, get the Linux kernel CFI warning free, then enable it for CFI builds by default. But that seems riskier to me (we are more likely to forget to come turn the warning on by default later).

I have sent patches to fix all the instances of this that I see in the Linux kernel as of this morning:

https://github.com/ClangBuiltLinux/linux/issues/1750#issuecomment-1300972689

I have to track all of those so I know when it is possible to enable this warning in KBUILD_CFLAGS so I could remind Sami to enable it for CFI builds by default once that is done?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).

Right, I was only mentioning our use case will necessitate it being always enabled so conditionally enabling it would be extra work that wouldn't benefit us but obviously, the world does not revolve around our one project :)

Phew, I'm glad I wasn't missing something subtle. :-D

8219

I have to track all of those so I know when it is possible to enable this warning in KBUILD_CFLAGS so I could remind Sami to enable it for CFI builds by default once that is done?

Okay, that seems reasonable enough to me (assuming @samitolvanen agrees, of course). If that's the route we're going, then I think this patch is basically ready to go as-is (the diagnostic is already off-by-default here) and the only thing left is the release note.

[samitolvanen]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

I have to track all of those so I know when it is possible to enable this warning in KBUILD_CFLAGS so I could remind Sami to enable it for CFI builds by default once that is done?

Note that this wouldn't only affect -fsanitize=kcfi. We still use -fsanitize=cfi in 5.13-6.0 arm64 kernels. We probably don't want to break those builds with ToT Clang either, so we might still need to disable the warning in the relevant stable trees (or backport the fixes there).

[samitolvanen]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

Okay, that seems reasonable enough to me (assuming @samitolvanen agrees, of course). If that's the route we're going, then I think this patch is basically ready to go as-is (the diagnostic is already off-by-default here) and the only thing left is the release note.

Sure, that works for me. I'll update the patch with release notes when I have a chance.

[nathanchance]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8219

I have to track all of those so I know when it is possible to enable this warning in KBUILD_CFLAGS so I could remind Sami to enable it for CFI builds by default once that is done?

Note that this wouldn't only affect -fsanitize=kcfi. We still use -fsanitize=cfi in 5.13-6.0 arm64 kernels. We probably don't want to break those builds with ToT Clang either, so we might still need to disable the warning in the relevant stable trees (or backport the fixes there).

Ah good point. It seems entirely reasonable to me to backport whatever fixes are needed for 5.15 and any other stable releases that are supported at the time, as they are fixing potential failures at run time. I can handle that before we enable it by default for either cfi and kcfi.

[samitolvanen]

Added release notes, grouped together with -Wcast-function-type-strict.

[aaron.ballman]

LGTM, thank you!

clang/docs/ReleaseNotes.rst

- Copy-elided initialization of lock scopes is now handled differently in
   which can result in new warnings (or make existing warnings disappear).
 - The wording of diagnostics regarding arithmetic on fixed-sized arrays and
   pointers is improved to include the type of the array and whether it's cast
   to another type. This should improve comprehension for why an index is
   out-of-bounds.
 - Clang now correctly points to the problematic parameter for the ``-Wnonnull``
   warning. This fixes
   `Issue 58273 <https://github.com/llvm/llvm-project/issues/58273>`_.
-- Introduced ``-Wcast-function-type-strict`` to warn about function type mismatches
-  in casts that may result in runtime indirect call `Control-Flow Integrity (CFI)
-  <https://clang.llvm.org/docs/ControlFlowIntegrity.html>`_ failures. This diagnostic
-  is grouped under ``-Wcast-function-type`` as it identifies a more strict set of
-  potentially problematic function type casts.
+- Introduced ``-Wcast-function-type-strict`` and
+  ``-Wincompatible-function-pointer-types-strict`` to warn about function type
+  mismatches in casts and assignments that may result in runtime indirect call
+  `Control-Flow Integrity (CFI)
+  <https://clang.llvm.org/docs/ControlFlowIntegrity.html>`_ failures. The
+  ``-Wcast-function-type-strict`` diagnostic is grouped under
+  ``-Wcast-function-type`` as it identifies a more strict set of potentially
+  problematic function type casts.
 - Clang will now disambiguate NTTP types when printing diagnostic that contain NTTP types.
   Fixes `Issue 57562 <https://github.com/llvm/llvm-project/issues/57562>`_.
 - Better error recovery for pack expansion of expressions.
   `Issue 58673 <https://github.com/llvm/llvm-project/issues/58673>`_.
 - Better diagnostics when the user has missed `auto` in a declaration.
   `Issue 49129 <https://github.com/llvm/llvm-project/issues/49129>`_.
 - Clang now diagnoses use of invalid or reserved module names in a module
   export declaration. Both are diagnosed as an error, but the diagnostic is

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_typecheck_convert_incompatible_function_pointer : Error<
   "|%diff{casting $ to type $|casting between types}0,1}2"
   "%select{|; dereference with *|"
   "; take the address with &|"
   "; remove *|"
   "; remove &}3">;
 def ext_typecheck_convert_incompatible_function_pointer : ExtWarn<
   err_typecheck_convert_incompatible_function_pointer.Text>,
   InGroup<IncompatibleFunctionPointerTypes>, DefaultError;
+def warn_typecheck_convert_incompatible_function_pointer_strict : Warning<
+  err_typecheck_convert_incompatible_function_pointer.Text>,
+  InGroup<DiagGroup<"incompatible-function-pointer-types-strict">>, DefaultIgnore;
 def ext_typecheck_convert_discards_qualifiers : ExtWarn<
   "%select{%diff{assigning to $ from $|assigning to different types}0,1"
   "|%diff{passing $ to parameter of type $|"
   "passing to parameter of different type}0,1"
   "|%diff{returning $ from a function with result type $|"
   "returning from function with different return type}0,1"
   "|%diff{converting $ to type $|converting between types}0,1"
   "|%diff{initializing $ with an expression of type $|"

clang/include/clang/Sema/Sema.h

enum AssignConvertType {
     /// are not compatible, but we accept them as an extension.
     IncompatiblePointer,
 
     /// IncompatibleFunctionPointer - The assignment is between two function
     /// pointers types that are not compatible, but we accept them as an
     /// extension.
     IncompatibleFunctionPointer,
 
+    /// IncompatibleFunctionPointerStrict - The assignment is between two
+    /// function pointer types that are not identical, but are compatible,
+    /// unless compiled with -fsanitize=cfi, in which case the type mismatch
+    /// may trip an indirect call runtime check.
+    IncompatibleFunctionPointerStrict,
+
     /// IncompatiblePointerSign - The assignment is between two pointers types
     /// which point to integers which have a different sign, but are otherwise
     /// identical. This is a subset of the above, but broken out because it's by
     /// far the most common case of incompatible pointers.
     IncompatiblePointerSign,
 
     /// CompatiblePointerDiscardsQualifiers - The assignment discards
     /// c/v/r qualifiers, which we accept as an extension.

clang/lib/Sema/SemaExpr.cpp

 }
 
 // checkPointerTypesForAssignment - This is a very tricky routine (despite
 // being closely modeled after the C99 spec:-). The odd characteristic of this
 // routine is it effectively iqnores the qualifiers on the top level pointee.
 // This circumvents the usual type rules specified in 6.2.7p1 & 6.7.5.[1-3].
 // FIXME: add a couple examples in this comment.
 static Sema::AssignConvertType
-checkPointerTypesForAssignment(Sema &S, QualType LHSType, QualType RHSType) {
+checkPointerTypesForAssignment(Sema &S, QualType LHSType, QualType RHSType,
+                               SourceLocation Loc) {
   assert(LHSType.isCanonical() && "LHS not canonicalized!");
   assert(RHSType.isCanonical() && "RHS not canonicalized!");
 
   // get the "pointed to" type (ignoring qualifiers at the top level)
   const Type *lhptee, *rhptee;
   Qualifiers lhq, rhq;
   std::tie(lhptee, lhq) =
       cast<PointerType>(LHSType)->getPointeeType().split().asPair();
if (rhptee->isVoidType()) {
     if (lhptee->isIncompleteOrObjectType())
       return ConvTy;
 
     // As an extension, we allow cast to/from void* to function pointer.
     assert(lhptee->isFunctionType());
     return Sema::FunctionVoidPointer;
   }
 
+  if (!S.Diags.isIgnored(
+          diag::warn_typecheck_convert_incompatible_function_pointer_strict,
+          Loc) &&
+      RHSType->isFunctionPointerType() && LHSType->isFunctionPointerType() &&
+      !S.IsFunctionConversion(RHSType, LHSType, RHSType))
+    return Sema::IncompatibleFunctionPointerStrict;
+
   // C99 6.5.16.1p1 (constraint 3): both operands are pointers to qualified or
   // unqualified versions of compatible types, ...
   QualType ltrans = QualType(lhptee, 0), rtrans = QualType(rhptee, 0);
   if (!S.Context.typesAreCompatible(ltrans, rtrans)) {
     // Check if the pointee types are compatible ignoring the sign.
     // We explicitly check for char so that we catch "char" vs
     // "unsigned char" on systems where "char" is unsigned.
     if (lhptee->isCharType())
if (isa<PointerType>(RHSType)) {
       LangAS AddrSpaceL = LHSPointer->getPointeeType().getAddressSpace();
       LangAS AddrSpaceR = RHSType->getPointeeType().getAddressSpace();
       if (AddrSpaceL != AddrSpaceR)
         Kind = CK_AddressSpaceConversion;
       else if (Context.hasCvrSimilarType(RHSType, LHSType))
         Kind = CK_NoOp;
       else
         Kind = CK_BitCast;
-      return checkPointerTypesForAssignment(*this, LHSType, RHSType);
+      return checkPointerTypesForAssignment(*this, LHSType, RHSType,
+                                            RHS.get()->getBeginLoc());
     }
 
     // int -> T*
     if (RHSType->isIntegerType()) {
       Kind = CK_IntegralToPointer; // FIXME: null?
       return IntToPointer;
     }
 
if (getLangOpts().CPlusPlus) {
       DiagKind = diag::err_typecheck_convert_int_pointer;
       isInvalid = true;
     } else {
       DiagKind = diag::ext_typecheck_convert_int_pointer;
     }
     ConvHints.tryToFixConversion(SrcExpr, SrcType, DstType, *this);
     MayHaveConvFixit = true;
     break;
+  case IncompatibleFunctionPointerStrict:
+    DiagKind =
+        diag::warn_typecheck_convert_incompatible_function_pointer_strict;
+    ConvHints.tryToFixConversion(SrcExpr, SrcType, DstType, *this);
+    MayHaveConvFixit = true;
+    break;
   case IncompatibleFunctionPointer:
     if (getLangOpts().CPlusPlus) {
       DiagKind = diag::err_typecheck_convert_incompatible_function_pointer;
       isInvalid = true;
     } else {
       DiagKind = diag::ext_typecheck_convert_incompatible_function_pointer;
     }
     ConvHints.tryToFixConversion(SrcExpr, SrcType, DstType, *this);

clang/test/Sema/incompatible-function-pointer-types-strict.c

+// RUN: %clang_cc1 -fsyntax-only %s -Wincompatible-function-pointer-types-strict -verify=soft,strict
+// RUN: %clang_cc1 -fsyntax-only %s -Werror=incompatible-function-pointer-types-strict -verify=hard,strict
+// RUN: %clang_cc1 -fsyntax-only %s -Wincompatible-function-pointer-types -verify=nonstrict
+// nonstrict-no-diagnostics
+
+enum E { A = -1, B };
+typedef enum E (*fn_a_t)(void);
+typedef void (*fn_b_t)(void);
+
+int a(void) { return 0; }
+void __attribute__((noreturn)) b(void) { while (1); }
+
+void fa(fn_a_t x) {} // strict-note {{passing argument to parameter 'x' here}}
+void fb(fn_b_t x) {}
+
+void baz(void) {
+  fa(&a); // soft-warning {{incompatible function pointer types passing 'int (*)(void)' to parameter of type 'fn_a_t' (aka 'enum E (*)(void)')}} \
+             hard-error {{incompatible function pointer types passing 'int (*)(void)' to parameter of type 'fn_a_t' (aka 'enum E (*)(void)')}}
+  fb(&b); // no-warning
+}