[UBSan] Disable the function sanitizer on an execute-only target.

[MaggieYi]

Summary

PR for https://github.com/llvm/llvm-project/issues/64931.

Disable the function and kcfi sanitizers on an execute-only target.

An execute-only target disallows data access to code sections. -fsanitize=function and -fsanitize=kcfi instrument indirect function calls to load a type hash before the function label. This results in a non-execute access to the code section and a runtime error.

To solve the issue, -fsanitize=function should not be included in any check group (e.g. undefined) on an execute-only target. If a user passes -fsanitize=undefined, there is no error and no warning. However, if the user explicitly passes -fsanitize=function or -fsanitize=kcfi on an execute-only target, an error will be emitted.

[probinson]

The PS5 bits LGTM, but as I'm not familiar with the ARM aspects I won't give final approval.

[simon_tatham]

I think this is also true of -fsanitize=kcfi, isn't it? That also works by writing a cookie just before the function entry point. If we're diagnosing incompatibilities with execute-only, then perhaps we should do it for both.

[simon_tatham]

clang/lib/Basic/Sanitizers.cpp

132–141

Why do we need to check both of -mexecute-only and the +execute-only target feature? It looks to me as if the code in Driver/ToolChains/Arch/ARM.cpp that handles -mexecute-only ends up setting the same target feature anyway. And it checks the supported architecture versions first.

Would it not be better to just check the target feature here, and avoid having a duplicated copy of the rest of this logic which needs to be kept in sync with the ARM driver?

Does something go wrong if you do it that way?

[MaskRay]

clang/include/clang/Basic/DiagnosticCommonKinds.td

326 ↗

(On Diff #552694)

We don't need this diagnostic as a common kind (we only use it in driver).

I think we can reuse err_drv_argument_not_allowed_with . Though for PS5 you will get ... allowed with '-mexecute-only' even if the user doesn't specify -mexecute-only, but I hope it is fine.

clang/lib/Basic/Sanitizers.cpp

132–141

I think we only need to check the +execute-only target feature and remove driver option -mexecute-only check.

if (Triple.isPS5())
  return true;
if (!Triple.isARM() && !Triple.isThumb())
  return false;
return features contains "+execute-only" ;

clang/lib/Driver/SanitizerArgs.cpp

407

I think it's clear not not to add the variable NotAllowedWithExecuteOnly.

Currently, I need to check the definition of NotAllowedWithExecuteOnly to understand that this comment does what it says. For now, just encoding Function here is clearer.

clang/lib/Frontend/CompilerInvocation.cpp

4405 ↗

(On Diff #552694)

Remove.

We don't perform rigid error checking for cc1. If the user bypass the driver check with -Xclang -fsanitize=function, we don't provide more diagnostics.

clang/test/CodeGen/ubsan-function.c

2 ↗

(On Diff #552694)

remove new tests. we only need test/Driver test.

[MaggieYi]

The changes include:

1. Added an ARM::supportedExecuteOnly function to avoid the duplicated code.
2. Modified the isExecuteOnlyTarget function to only deal with the -mexecute-only option.
3. Added SanitizerKind::KCFI to the SanitizerMask of NotAllowedWithExecuteOnly.

[MaggieYi]

Thanks @simon_tatham and @MaskRay for the quick code review.

When I work on this issue, I want to add an error for both clang and clang -cc1.
-mexecute-only is an ARM-only compiler option. The clang Driver will convert -mexecute-only to -target-feature +execute-only in the clang cc1 command.
To pass the execute-only option, the clang command is: clang -target=armv6t2-eabi -mexecute-only …. The Arm clang cc1 command:
clang -cc1 -triple armv6t2-unknown-unknown-eabi -target-feature +execute-only….

If I move the change to the code in Driver/ToolChains/Arch/ARM.cpp, the error will only deal with the -mexecute-only option, but not handle the -target-feature +execute-only in the clang -cc1 command.

As @MaskRay commented that we don't perform rigid error checking for cc1. In this case, we could handle the error in the Driver/ToolChains/Arch/ARM.cpp. However, the target-specific predicate function (named isExecuteOnlyTarget) allows any other targets that support execute-only could get the effect by modifying just the isExecuteOnlyTarget function. Therefore, I have modified the isExecuteOnlyTarget function to only deal with the -mexecute-only option. I have also added a function (named ARM::supportedExecuteOnly) to avoid the duplicated code.

clang/include/clang/Basic/DiagnosticCommonKinds.td

326 ↗

(On Diff #552694)

Since err_drv_argument_not_allowed_with is an ARM-only option, We cannot reuse it.

clang/lib/Driver/SanitizerArgs.cpp

407

NotAllowedWithExecuteOnly is modified to include the SanitizerKind::KCFI, therefore I keep it in the code.

[MaskRay]

clang/include/clang/Basic/DiagnosticCommonKinds.td

326 ↗

(On Diff #552694)

err_drv_argument_not_allowed_with is a generic diagnostic. My point is that we don't need an extra err_unsupported_opt_for_execute_only_target.

clang/lib/Basic/Sanitizers.cpp

125

The idiom is hasFlag(clang::driver::options::OPT_mexecute_only, clang::driver::options::OPT_mno_execute_only, false)

129

I don't think we need an extra llvm::ARM::supportedExecuteOnly check. We just return true when -mexecute-only is in effect.

clang/lib/Driver/SanitizerArgs.cpp

478

`-fsanitize=function => NotAllowedWithExecuteOnly

since we now handle kcfi as well.

481

omit braces

clang/test/Driver/fsanitize.c

977

Drop not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined -fsanitize=function.
Testing just -fsanitize=function for the negative test is sufficient.

979

Drop -fsanitize=function -fsanitize=kcfi line. They already lead to an error.

980

--target=armv6t2-eabi -mexecute-only -fsanitize=undefined needs a custom check prefix that checks function is not enabled.

llvm/lib/TargetParser/ARMTargetParser.cpp

602 ↗

(On Diff #553409)

We don't need to extract the function.

[MaggieYi]

Thanks @MaskRay, I have updated the patch following your suggestions.

clang/include/clang/Basic/DiagnosticCommonKinds.td

326 ↗

(On Diff #552694)

Sorry, I mean that -mexecute-only is the arm-only option Options.td#L4202.

As you suggested that we could use err_drv_argument_not_allowed_with by passing -fsanitize=function (not -mexecute-only) and the target triple. For example, "error: invalid argument '-fsanitize=function' not allowed with 'x86_64-sie-ps5'". I have removed err_unsupported_opt_for_execute_only_target.

[MaskRay]

Thanks!

clang/lib/Basic/Sanitizers.cpp

125

return Args.hasFlag(clang::driver::options::OPT_mexecute_only ...

clang/test/Driver/fsanitize.c

982

Delete this -NOT. %clang returning 0 implies that there is no error.

[MaggieYi]

Thanks @MaskRay. The patch is updated.

Hi @simon_tatham, are you happy with the patch?

Thanks

[simon_tatham]

Yes, this looks good to me too. Thanks!

[MaskRay]

clang/lib/Basic/Sanitizers.cpp

129

Sorry, I just noticed that this adds a circular (if we consider headers) dependency from clangBasic to clangDriver. I'm removing it.

[MaggieYi]

Thanks @MaskRay.

clang/include/clang/Basic/Sanitizers.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/HashBuilder.h"
 #include "llvm/Transforms/Instrumentation/AddressSanitizerOptions.h"
 #include <cassert>
 #include <cstdint>
 
 namespace llvm {
 class hash_code;
+class Triple;
+namespace opt {
+class ArgList;
 }
+} // namespace llvm
 
 namespace clang {
 
 class SanitizerMask {
   // NOTE: this class assumes kNumElem == 2 in most of the constexpr functions,
   // in order to work within the C++11 constexpr function constraints. If you
   // change kNumElem, you'll need to update those member functions as well.
 

 llvm::AsanDtorKind AsanDtorKindFromString(StringRef kind);
 
 StringRef AsanDetectStackUseAfterReturnModeToString(
     llvm::AsanDetectStackUseAfterReturnMode mode);
 
 llvm::AsanDetectStackUseAfterReturnMode
 AsanDetectStackUseAfterReturnModeFromString(StringRef modeStr);
 
+/// Return true if an execute-only target disallows data access to code
+/// sections.
+bool isExecuteOnlyTarget(const llvm::Triple &Triple,
+                         const llvm::opt::ArgList &Args);
+
 } // namespace clang
 
 #endif // LLVM_CLANG_BASIC_SANITIZERS_H

clang/lib/Basic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
+  Option
   Support
   TargetParser
   )
 
 find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${CLANG_SOURCE_DIR}" clang_vc)
 
 # The VC revision include that we want to generate.

clang/lib/Basic/Sanitizers.cpp

 //===- Sanitizers.cpp - C Language Family Language Options ----------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the classes from Sanitizers.h
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/Sanitizers.h"
+#include "clang/Driver/Options.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Option/ArgList.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/TargetParser/Triple.h"
 
 using namespace clang;
 
 // Once LLVM switches to C++17, the constexpr variables can be inline and we
 // won't need this.
 #define SANITIZER(NAME, ID) constexpr SanitizerMask SanitizerKind::ID;
 #define SANITIZER_GROUP(NAME, ID, ALIAS)                                       \
   constexpr SanitizerMask SanitizerKind::ID;                                   \

 AsanDetectStackUseAfterReturnModeFromString(StringRef modeStr) {
   return llvm::StringSwitch<llvm::AsanDetectStackUseAfterReturnMode>(modeStr)
       .Case("always", llvm::AsanDetectStackUseAfterReturnMode::Always)
       .Case("runtime", llvm::AsanDetectStackUseAfterReturnMode::Runtime)
       .Case("never", llvm::AsanDetectStackUseAfterReturnMode::Never)
       .Default(llvm::AsanDetectStackUseAfterReturnMode::Invalid);
 }
 
+bool isExecuteOnlyTarget(const llvm::Triple &Triple,
+                         const llvm::opt::ArgList &Args) {
+  if (Triple.isPS5())
+    return true;
+
+  // On Arm, the clang `-mexecute-only` option is used to generate the
+  // execute-only output (no data access to code sections).
+  return Args.hasFlag(clang::driver::options::OPT_mexecute_only,
+                      clang::driver::options::OPT_mno_execute_only, false);
+}
 } // namespace clang

clang/lib/Driver/SanitizerArgs.cpp

static const SanitizerMask NeedsUbsanRt =
     SanitizerKind::Undefined | SanitizerKind::Integer |
     SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
     SanitizerKind::CFI | SanitizerKind::FloatDivideByZero |
     SanitizerKind::ObjCCast;
 static const SanitizerMask NeedsUbsanCxxRt =
     SanitizerKind::Vptr | SanitizerKind::CFI;
 static const SanitizerMask NotAllowedWithTrap = SanitizerKind::Vptr;
 static const SanitizerMask NotAllowedWithMinimalRuntime = SanitizerKind::Vptr;
+static const SanitizerMask NotAllowedWithExecuteOnly =
+    SanitizerKind::Function | SanitizerKind::KCFI;
 static const SanitizerMask RequiresPIE =
     SanitizerKind::DataFlow | SanitizerKind::Scudo;
 static const SanitizerMask NeedsUnwindTables =
     SanitizerKind::Address | SanitizerKind::HWAddress | SanitizerKind::Thread |
     SanitizerKind::Memory | SanitizerKind::DataFlow;
 static const SanitizerMask SupportsCoverage =
     SanitizerKind::Address | SanitizerKind::HWAddress |
     SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress |
if (Arg->getOption().matches(options::OPT_fsanitize_EQ)) {
           if (DiagnoseErrors)
             D.Diag(diag::err_drv_argument_only_allowed_with)
                 << "-fsanitize=function"
                 << "-mcmodel=small";
           Add &= ~SanitizerKind::Function;
           DiagnosedKinds |= SanitizerKind::Function;
         }
       }
+      // -fsanitize=function and -fsanitize=kcfi instrument indirect function
+      // calls to load a type hash before the function label. Therefore, an
+      // execute-only target doesn't support the function and kcfi sanitizers.
+      const llvm::Triple &Triple = TC.getTriple();
+      if (isExecuteOnlyTarget(Triple, Args)) {
+        if (SanitizerMask KindsToDiagnose =
+                Add & NotAllowedWithExecuteOnly & ~DiagnosedKinds) {
+          if (DiagnoseErrors) {
+            std::string Desc = describeSanitizeArg(Arg, KindsToDiagnose);
+            D.Diag(diag::err_drv_argument_not_allowed_with)
+                << Desc << Triple.str();
+          }
+          DiagnosedKinds |= KindsToDiagnose;
+        }
+        Add &= ~NotAllowedWithExecuteOnly;
+      }
 
       // FIXME: Make CFI on member function calls compatible with cross-DSO CFI.
       // There are currently two problems:
       // - Virtual function call checks need to pass a pointer to the function
       //   address to llvm.type.test and a pointer to the address point to the
       //   diagnostic function. Currently we pass the same pointer to both
       //   places.
       // - Non-virtual function call checks may need to check multiple type
if (Arg->getOption().matches(options::OPT_fsanitize_EQ)) {
       // Group expansion may have enabled a sanitizer which is disabled later.
       Add &= ~AllRemove;
       // Silently discard any unsupported sanitizers implicitly enabled through
       // group expansion.
       Add &= ~InvalidTrappingKinds;
       if (MinimalRuntime) {
         Add &= ~NotAllowedWithMinimalRuntime;
       }
+      // NotAllowedWithExecuteOnly is silently discarded on an execute-only
+      // target if implicitly enabled through group expansion.
+      if (isExecuteOnlyTarget(Triple, Args))
+        Add &= ~NotAllowedWithExecuteOnly;
       if (CfiCrossDso)
         Add &= ~SanitizerKind::CFIMFCall;
       Add &= Supported;
 
       if (Add & SanitizerKind::Fuzzer)
         Add |= SanitizerKind::FuzzerNoLink;
 
       // Enable coverage if the fuzzing flag is set.

clang/test/CodeGenObjCXX/crash-function-type.mm

+// Mark test as unsupported on PS5 due to PS5 doesn't support function sanitizer.
+// UNSUPPORTED: target=x86_64-sie-ps5
+
 // RUN: %clang_cc1 -fblocks -fsanitize=function -emit-llvm %s -o %t
 
 void g(void (^)());
 void f() {
   __block int a = 0;
   g(^() {
     a++;
   });
 }

clang/test/Driver/fsanitize.c

 // RUN: %clang -fsanitize=float-divide-by-zero -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-MINIMAL
 // CHECK-DIVBYZERO-MINIMAL: "-fsanitize-minimal-runtime"
 
 // RUN: %clang -fsanitize=undefined,float-divide-by-zero %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-DIVBYZERO-UBSAN
 // CHECK-DIVBYZERO-UBSAN: "-fsanitize={{.*}},float-divide-by-zero,{{.*}}"
 
 // RUN: not %clang --target=x86_64-linux-gnu -fsanitize=undefined,function -mcmodel=large %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION-CODE-MODEL
 // CHECK-UBSAN-FUNCTION-CODE-MODEL: error: invalid argument '-fsanitize=function' only allowed with '-mcmodel=small'
+
+// RUN: not %clang --target=x86_64-sie-ps5 -fsanitize=function %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: not %clang --target=x86_64-sie-ps5 -fsanitize=undefined -fsanitize=function %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: not %clang --target=x86_64-sie-ps5 -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI
+// RUN: not %clang --target=x86_64-sie-ps5 -fsanitize=function -fsanitize=kcfi %s -### 2>&1 | FileCheck %s  --check-prefix=CHECK-UBSAN-KCFI --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: %clang --target=x86_64-sie-ps5 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED
+
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=function %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI
+// RUN: %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED
+
+// CHECK-UBSAN-KCFI-DAG: error: invalid argument '-fsanitize=kcfi' not allowed with {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
+// CHECK-UBSAN-FUNCTION-DAG: error: invalid argument '-fsanitize=function' not allowed with {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
+// CHECK-UBSAN-UNDEFINED: "-fsanitize={{((alignment|array-bounds|bool|builtin|enum|float-cast-overflow|integer-divide-by-zero|nonnull-attribute|null|pointer-overflow|return|returns-nonnull-attribute|shift-base|shift-exponent|signed-integer-overflow|unreachable|vla-bound),?){17}"}}