[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

133–142

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

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

133–142

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

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

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

126

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

130

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

981

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

983

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

984

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

llvm/lib/TargetParser/ARMTargetParser.cpp

602

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

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

126

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

clang/test/Driver/fsanitize.c

986

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

130

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/DiagnosticCommonKinds.td

 def note_valid_options : Note<"valid target CPU values are: %0">;
 def err_target_unsupported_cpu_for_micromips : Error<
   "micromips is not supported for target CPU '%0'">;
 def err_target_unknown_abi : Error<"unknown target ABI '%0'">;
 def err_target_unsupported_abi : Error<"ABI '%0' is not supported on CPU '%1'">;
 def err_target_unsupported_abi_for_triple : Error<
   "ABI '%0' is not supported for '%1'">;
 def err_unsupported_abi_for_opt : Error<"'%0' can only be used with the '%1' ABI">;
+def err_unsupported_opt_for_execute_only_target
+    : Error<"unsupported option '%0' for the execute only target '%1'">;
 def err_mips_fp64_req : Error<
     "'%0' can only be used if the target supports the mfhc1 and mthc1 instructions">;
 def err_target_unknown_fpmath : Error<"unknown FP unit '%0'">;
 def err_target_unsupported_fpmath : Error<
     "the '%0' unit is not supported with this instruction set">;
 def err_target_unsupported_unaligned : Error<
   "the %0 sub-architecture does not support unaligned accesses">;
 def err_target_unsupported_execute_only : Error<

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/ARMTargetParser.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).
+  if (const llvm::opt::Arg *A =
+          Args.getLastArg(clang::driver::options::OPT_mexecute_only,
+                          clang::driver::options::OPT_mno_execute_only)) {
+    if (A->getOption().matches(clang::driver::options::OPT_mexecute_only) &&
+        llvm::ARM::supportedExecuteOnly(Triple)) {
+      return true;
+    }
+  }
+
+  return 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_unsupported_opt_for_execute_only_target)
+                << 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;
       }
+      // `-fsanitize=function` 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/lib/Driver/ToolChains/Arch/ARM.cpp

fp16_fml_fallthrough:
   // This only makes sense for the compiler, not for the assembler.
   // It's not needed for multilib selection and may hide an unused
   // argument diagnostic if the code is always run.
   if (!ForAS && !ForMultilib) {
     // Supported only on ARMv6T2 and ARMv7 and above.
     // Cannot be combined with -mno-movt.
     if (Arg *A = Args.getLastArg(options::OPT_mexecute_only, options::OPT_mno_execute_only)) {
       if (A->getOption().matches(options::OPT_mexecute_only)) {
-        if (getARMSubArchVersionNumber(Triple) < 7 &&
-            llvm::ARM::parseArch(Triple.getArchName()) != llvm::ARM::ArchKind::ARMV6T2 &&
-            llvm::ARM::parseArch(Triple.getArchName()) != llvm::ARM::ArchKind::ARMV6M)
-              D.Diag(diag::err_target_unsupported_execute_only) << Triple.getArchName();
+        if (!llvm::ARM::supportedExecuteOnly(Triple))
+          D.Diag(diag::err_target_unsupported_execute_only)
+              << Triple.getArchName();
         else if (llvm::ARM::parseArch(Triple.getArchName()) == llvm::ARM::ArchKind::ARMV6M) {
           if (Arg *PIArg = Args.getLastArg(options::OPT_fropi, options::OPT_frwpi,
                                            options::OPT_fpic, options::OPT_fpie,
                                            options::OPT_fPIC, options::OPT_fPIE))
             D.Diag(diag::err_opt_not_valid_with_opt_on_target)
                 << A->getAsString(Args) << PIArg->getAsString(Args) << Triple.getArchName();
         } else if (Arg *B = Args.getLastArg(options::OPT_mno_movt))
           D.Diag(diag::err_opt_not_valid_with_opt)

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=undefined -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: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=function -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED
+
+// CHECK-UBSAN-KCFI-DAG: error: unsupported option '-fsanitize=kcfi' for the execute only target {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
+// CHECK-UBSAN-FUNCTION-DAG: error: unsupported option '-fsanitize=function' for the execute only target {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
+// CHECK-UBSAN-UNDEFINED-NOT: error: unsupported option '-fsanitize=function' for the execute only target {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}

llvm/include/llvm/TargetParser/ARMTargetParser.h

 StringRef computeDefaultTargetABI(const Triple &TT, StringRef CPU);
 
 /// Get the (LLVM) name of the minimum ARM CPU for the arch we are targeting.
 ///
 /// \param Arch the architecture name (e.g., "armv7s"). If it is an empty
 /// string then the triple's arch name is used.
 StringRef getARMCPUForArch(const llvm::Triple &Triple, StringRef MArch = {});
 
+/// Return true if the arch supports the execute-only output. Currently, the
+/// execute-only output is only supported on ARMv6T2 and ARMv7 and above.
+bool supportedExecuteOnly(const Triple &TT);
 } // namespace ARM
 } // namespace llvm
 
 #endif

llvm/lib/TargetParser/ARMTargetParser.cpp

case llvm::Triple::MuslEABIHF:
       return "arm1176jzf-s";
     default:
       return "arm7tdmi";
     }
   }
 
   llvm_unreachable("invalid arch name");
 }
+
+bool ARM::supportedExecuteOnly(const Triple &TT) {
+  if (parseArchVersion(TT.getArchName()) < 7 &&
+      parseArch(TT.getArchName()) != ArchKind::ARMV6T2 &&
+      parseArch(TT.getArchName()) != ArchKind::ARMV6M)
+    return false;
+  return true;
+}