Revert "Ensure Branch Target Indentification is enabled for executabl…

…e pages."

This reverts commit 152f6cb.

Reason for revert: breaks the mac-arm64-rel bot

Original change's description:
> Ensure Branch Target Indentification is enabled for executable pages.
>
> Adds unit tests which verify that pages allocated via the allocator of
> V8Platform have proper BTI protection enabled if permissions are set to
> anything executable.
>
> The test tries to verify correct behaviour required in case of BTI
> violations rather than merely checking that protection flags are set.
>
> Bug: 1145581
> Change-Id: I900cd0c334f5e50868d6972e40fc9fa6261b05a8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3121068
> Reviewed-by: Wez <wez@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Commit-Queue: Richard Townsend <richard.townsend@arm.com>
> Cr-Commit-Position: refs/heads/main@{#922078}

Bug: 1145581
Change-Id: Ie2d8e923687d2b701964382f82007c32b7a95a77
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3164326
Auto-Submit: Richard Townsend <richard.townsend@arm.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Omar Morsi <omorsi@google.com>
Owners-Override: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/main@{#922084}

base/BUILD.gn

@@ -2890,18 +2890,9 @@ source_set("base_unittests_tasktraits") {
   deps = [ ":base" ]
 }
 
-source_set("arm_bti_testfunctions") {
-  testonly = true
-
-  sources = [ "allocator/partition_allocator/arm_bti_test_functions.h" ]
-
-  if (target_cpu == "arm64") {
-    sources += [ "allocator/partition_allocator/arm_bti_test_functions.S" ]
-  }
-}
-
 test("base_unittests") {
   sources = [
+    "allocator/partition_allocator/arm_bti_test_functions.h",
     "allocator/tcmalloc_unittest.cc",
     "as_const_unittest.cc",
     "at_exit_unittest.cc",
@@ -3227,7 +3218,6 @@ test("base_unittests") {
   defines = []
 
   deps = [
-    ":arm_bti_testfunctions",
     ":base",
     ":base_stack_sampling_profiler_test_util",
     ":base_unittests_tasktraits",
@@ -3698,6 +3688,10 @@ test("base_unittests") {
     ]
   }
 
+  if (target_cpu == "arm64" && (is_linux || is_android)) {
+    sources += [ "allocator/partition_allocator/arm_bti_test_functions.S" ]
+  }
+
   configs += [
     ":memory_tagging",
     "//build/config/compiler:noshadowing",

base/allocator/partition_allocator/arm_bti_test_functions.S

@@ -33,9 +33,6 @@ arm_bti_test_function_invalid_offset:
 arm_bti_test_function_end:
     nop
 
-// For details see section "6.2 Program Property" in
-// "ELF for the Arm 64-bit Architecture (AArch64)"
-// https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#62program-property
 .pushsection .note.gnu.property, "a";
 .balign 8;
 .long 4;

base/allocator/partition_allocator/arm_bti_test_functions.h

@@ -6,24 +6,10 @@
 #define BASE_ALLOCATOR_PARTITION_ALLOCATOR_ARM_BTI_TEST_FUNCTIONS_H_
 
 #include "build/build_config.h"
-
 #if defined(ARCH_CPU_ARM64)
 extern "C" {
-/**
- * A valid BTI function. Jumping to this funtion should not cause any problem in
- * a BTI enabled environment.
- **/
 int64_t arm_bti_test_function(int64_t);
-
-/**
- * A function without proper BTI landing pad. Jumping here should crash the
- * program on systems which support BTI.
- **/
 int64_t arm_bti_test_function_invalid_offset(int64_t);
-
-/**
- * A simple function which immediately returns to sender.
- **/
 void arm_bti_test_function_end(void);
 }
 #endif  // defined(ARCH_CPU_ARM64)

base/allocator/partition_allocator/page_allocator_internals_posix.cc

@@ -15,18 +15,38 @@
 
 namespace base {
 
+// Two helper functions to detect whether we can safely use PROT_BTI
+// and PROT_MTE (static CPU triggers a -Wexit-time-destructors warning.)
+static bool HasCPUBranchIdentification() {
+#if defined(ARCH_CPU_ARM_FAMILY)
+  CPU cpu = CPU::CreateNoAllocation();
+  return cpu.has_bti();
+#else
+  return false;
+#endif
+}
+
+static bool HasCPUMemoryTaggingExtension() {
+#if defined(ARCH_CPU_ARM_FAMILY)
+  CPU cpu = CPU::CreateNoAllocation();
+  return cpu.has_mte();
+#else
+  return false;
+#endif
+}
+
 int GetAccessFlags(PageAccessibilityConfiguration accessibility) {
+  static const bool has_bti = HasCPUBranchIdentification();
+  static const bool has_mte = HasCPUMemoryTaggingExtension();
   switch (accessibility) {
     case PageRead:
       return PROT_READ;
     case PageReadWrite:
       return PROT_READ | PROT_WRITE;
     case PageReadWriteTagged:
-      return PROT_READ | PROT_WRITE |
-             (CPU::GetInstanceNoAllocation().has_mte() ? PROT_MTE : 0u);
+      return PROT_READ | PROT_WRITE | (has_mte ? PROT_MTE : 0u);
     case PageReadExecuteProtected:
-      return PROT_READ | PROT_EXEC |
-             (CPU::GetInstanceNoAllocation().has_bti() ? PROT_BTI : 0u);
+      return PROT_READ | PROT_EXEC | (has_bti ? PROT_BTI : 0u);
     case PageReadExecute:
       return PROT_READ | PROT_EXEC;
     case PageReadWriteExecute:

base/allocator/partition_allocator/page_allocator_unittest.cc

@@ -215,29 +215,27 @@ TEST(PartitionAllocPageAllocatorTest,
   }
 #if defined(MTE_KILLED_BY_SIGNAL_AVAILABLE)
   // Next, map some read-write memory and copy the BTI-enabled function there.
-  char* const buffer = reinterpret_cast<char*>(AllocPages(
-      nullptr, PageAllocationGranularity(), PageAllocationGranularity(),
-      PageReadWrite, PageTag::kChromium));
+  void* buffer = AllocPages(nullptr, PageAllocationGranularity(),
+                            PageAllocationGranularity(), PageReadWrite,
+                            PageTag::kChromium);
   ptrdiff_t function_range =
-      reinterpret_cast<char*>(arm_bti_test_function_end) -
-      reinterpret_cast<char*>(arm_bti_test_function);
+      reinterpret_cast<ptrdiff_t>(arm_bti_test_function_end) -
+      reinterpret_cast<ptrdiff_t>(arm_bti_test_function);
   ptrdiff_t invalid_offset =
-      reinterpret_cast<char*>(arm_bti_test_function_invalid_offset) -
-      reinterpret_cast<char*>(arm_bti_test_function);
+      reinterpret_cast<ptrdiff_t>(arm_bti_test_function_invalid_offset) -
+      reinterpret_cast<ptrdiff_t>(arm_bti_test_function);
   memcpy(buffer, reinterpret_cast<void*>(arm_bti_test_function),
          function_range);
-
+  uint32_t* bufferi = reinterpret_cast<uint32_t*>(buffer);
   // Next re-protect the page.
   SetSystemPagesAccess(buffer, PageAllocationGranularity(),
                        PageReadExecuteProtected);
-
-  using BTITestFunction = int64_t (*)(int64_t);
-
   // Attempt to call the function through the BTI-enabled entrypoint. Confirm
   // that it works.
-  BTITestFunction bti_enabled_fn = reinterpret_cast<BTITestFunction>(buffer);
-  BTITestFunction bti_invalid_fn =
-      reinterpret_cast<BTITestFunction>(buffer + invalid_offset);
+  int64_t (*bti_enabled_fn)(int64_t) =
+      reinterpret_cast<int64_t (*)(int64_t)>(bufferi);
+  int64_t (*bti_invalid_fn)(int64_t) =
+      reinterpret_cast<int64_t (*)(int64_t)>(bufferi + invalid_offset);
   EXPECT_EQ(bti_enabled_fn(15), 18);
   // Next, attempt to call the function without the entrypoint.
   EXPECT_EXIT({ bti_invalid_fn(15); }, testing::KilledBySignal(SIGILL),

base/cpu.cc

@@ -15,12 +15,12 @@
 #include <utility>
 
 #include "base/cxx17_backports.h"
-#include "base/no_destructor.h"
 
 #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_ANDROID) || \
     defined(OS_AIX)
 #include "base/containers/flat_set.h"
 #include "base/files/file_util.h"
+#include "base/no_destructor.h"
 #include "base/notreached.h"
 #include "base/process/internal_linux.h"
 #include "base/strings/string_number_conversions.h"
@@ -636,10 +636,4 @@ bool CPU::GetCumulativeCoreIdleTimes(CoreIdleTimes& idle_times) {
 #endif  // defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_ANDROID) ||
         // defined(OS_AIX)
 
-const CPU& CPU::GetInstanceNoAllocation() {
-  static const base::NoDestructor<const CPU> cpu(CPU(false));
-
-  return *cpu;
-}
-
 }  // namespace base

base/cpu.h

@@ -37,12 +37,12 @@ class BASE_EXPORT CPU final {
   CPU();
   CPU(CPU&&);
   CPU(const CPU&) = delete;
-
-  // Get a preallocated instance of CPU.
-  // This can be used in very early application startup. The instance of CPU is
-  // created without branding, see CPU(bool requires_branding) for details and
-  // implications.
-  static const CPU& GetInstanceNoAllocation();
+  // Construction path used in very early application startup. The difference
+  // between this and CPU::CPU() is that this doesn't allocate any memory, the
+  // catch is that no CPU model information is available (only features).
+#if defined(ARCH_CPU_ARM_FAMILY)
+  static CPU CreateNoAllocation() { return CPU(false); }
+#endif
 
   enum IntelMicroArchitecture {
     PENTIUM,
@@ -90,13 +90,8 @@ class BASE_EXPORT CPU final {
   uint32_t part_number() const { return part_number_; }
 
   // Armv8.5-A extensions for control flow and memory safety.
-#if defined(ARCH_CPU_ARM_FAMILY)
   bool has_mte() const { return has_mte_; }
   bool has_bti() const { return has_bti_; }
-#else
-  constexpr bool has_mte() const { return false; }
-  constexpr bool has_bti() const { return false; }
-#endif
 
   IntelMicroArchitecture GetIntelMicroArchitecture() const;
   const std::string& cpu_brand() const { return cpu_brand_; }
@@ -180,10 +175,8 @@ class BASE_EXPORT CPU final {
   bool has_avx_ = false;
   bool has_avx2_ = false;
   bool has_aesni_ = false;
-#if defined(ARCH_CPU_ARM_FAMILY)
   bool has_mte_ = false;  // Armv8.5-A MTE (Memory Taggging Extension)
   bool has_bti_ = false;  // Armv8.5-A BTI (Branch Target Identification)
-#endif
   bool has_non_stop_time_stamp_counter_ = false;
   bool is_running_in_vm_ = false;
   std::string cpu_vendor_ = "unknown";

gin/BUILD.gn

@@ -2,7 +2,6 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import("//base/allocator/allocator.gni")
 import("//testing/test.gni")
 import("//tools/v8_context_snapshot/v8_context_snapshot.gni")
 import("//v8/gni/v8.gni")
@@ -72,13 +71,6 @@ component("gin") {
     "wrapper_info.cc",
   ]
 
-  if (use_partition_alloc) {
-    sources += [
-      "v8_platform_page_allocator.cc",
-      "v8_platform_page_allocator.h",
-    ]
-  }
-
   if (v8_use_external_startup_data) {
     data = [ "$root_out_dir/snapshot_blob.bin" ]
   }
@@ -159,11 +151,6 @@ test("gin_unittests") {
     "//v8",
   ]
 
-  if (use_partition_alloc) {
-    sources += [ "v8_platform_page_allocator_unittest.cc" ]
-    deps += [ "//base:arm_bti_testfunctions" ]
-  }
-
   configs += [
     "//tools/v8_context_snapshot:use_v8_context_snapshot",
     "//v8:external_startup_data",

gin/public/v8_platform.h

@@ -9,7 +9,6 @@
 #include "base/compiler_specific.h"
 #include "base/lazy_instance.h"
 #include "gin/gin_export.h"
-#include "gin/v8_platform_page_allocator.h"
 #include "v8/include/v8-platform.h"
 
 namespace gin {
@@ -25,14 +24,9 @@ class GIN_EXPORT V8Platform : public v8::Platform {
 // v8::Platform implementation.
 // Some configurations do not use page_allocator.
 #if BUILDFLAG(USE_PARTITION_ALLOC)
-  // GetPageAllocator returns gin::PageAllocator instead of v8::PageAllocator,
-  // so we can be sure that the allocator used employs security features such as
-  // enabling Arm's Branch Target Instructions for executable pages. This is
-  // verified in the tests for gin::PageAllocator.
-  PageAllocator* GetPageAllocator() override;
+  v8::PageAllocator* GetPageAllocator() override;
   void OnCriticalMemoryPressure() override;
 #endif
-
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
       v8::Isolate*) override;
   int NumberOfWorkerThreads() override;