Reland "Ensure Branch Target Identification is enabled for executable…

… pages."

This is a reland of 152f6cb

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: I35a15cc28a4f63d7c2eb23cd9c5521ce28f4a75a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3162087
Commit-Queue: Richard Townsend <richard.townsend@arm.com>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Owners-Override: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#922488}

base/BUILD.gn

@@ -2892,9 +2892,21 @@ source_set("base_unittests_tasktraits") {
   deps = [ ":base" ]
 }
 
+source_set("arm_bti_testfunctions") {
+  testonly = true
+
+  sources = []
+
+  if (target_cpu == "arm64" && (is_linux || is_android)) {
+    sources = [
+      "allocator/partition_allocator/arm_bti_test_functions.S",
+      "allocator/partition_allocator/arm_bti_test_functions.h",
+    ]
+  }
+}
+
 test("base_unittests") {
   sources = [
-    "allocator/partition_allocator/arm_bti_test_functions.h",
     "allocator/tcmalloc_unittest.cc",
     "as_const_unittest.cc",
     "at_exit_unittest.cc",
@@ -3221,6 +3233,7 @@ test("base_unittests") {
   defines = []
 
   deps = [
+    ":arm_bti_testfunctions",
     ":base",
     ":base_stack_sampling_profiler_test_util",
     ":base_unittests_tasktraits",
@@ -3691,10 +3704,6 @@ 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,6 +33,9 @@ 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,10 +6,24 @@
 #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,38 +15,18 @@
 
 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 | (has_mte ? PROT_MTE : 0u);
+      return PROT_READ | PROT_WRITE |
+             (CPU::GetInstanceNoAllocation().has_mte() ? PROT_MTE : 0u);
     case PageReadExecuteProtected:
-      return PROT_READ | PROT_EXEC | (has_bti ? PROT_BTI : 0u);
+      return PROT_READ | PROT_EXEC |
+             (CPU::GetInstanceNoAllocation().has_bti() ? PROT_BTI : 0u);
     case PageReadExecute:
       return PROT_READ | PROT_EXEC;
     case PageReadWriteExecute:

base/allocator/partition_allocator/page_allocator_unittest.cc

@@ -215,27 +215,29 @@ TEST(PartitionAllocPageAllocatorTest,
   }
 #if defined(MTE_KILLED_BY_SIGNAL_AVAILABLE)
   // Next, map some read-write memory and copy the BTI-enabled function there.
-  void* buffer = AllocPages(nullptr, PageAllocationGranularity(),
-                            PageAllocationGranularity(), PageReadWrite,
-                            PageTag::kChromium);
+  char* const buffer = reinterpret_cast<char*>(AllocPages(
+      nullptr, PageAllocationGranularity(), PageAllocationGranularity(),
+      PageReadWrite, PageTag::kChromium));
   ptrdiff_t function_range =
-      reinterpret_cast<ptrdiff_t>(arm_bti_test_function_end) -
-      reinterpret_cast<ptrdiff_t>(arm_bti_test_function);
+      reinterpret_cast<char*>(arm_bti_test_function_end) -
+      reinterpret_cast<char*>(arm_bti_test_function);
   ptrdiff_t invalid_offset =
-      reinterpret_cast<ptrdiff_t>(arm_bti_test_function_invalid_offset) -
-      reinterpret_cast<ptrdiff_t>(arm_bti_test_function);
+      reinterpret_cast<char*>(arm_bti_test_function_invalid_offset) -
+      reinterpret_cast<char*>(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.
-  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);
+  BTITestFunction bti_enabled_fn = reinterpret_cast<BTITestFunction>(buffer);
+  BTITestFunction bti_invalid_fn =
+      reinterpret_cast<BTITestFunction>(buffer + 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,4 +636,10 @@ 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;
-  // 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
+
+  // 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();
 
   enum IntelMicroArchitecture {
     PENTIUM,
@@ -90,8 +90,13 @@ 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_; }
@@ -175,8 +180,10 @@ 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,6 +2,7 @@
 # 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")
@@ -71,6 +72,13 @@ 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" ]
   }
@@ -151,6 +159,13 @@ test("gin_unittests") {
     "//v8",
   ]
 
+  if (use_partition_alloc) {
+    sources += [ "v8_platform_page_allocator_unittest.cc" ]
+    if (target_cpu == "arm64" && (is_linux || is_android)) {
+      deps += [ "//base:arm_bti_testfunctions" ]
+    }
+  }
+
   configs += [
     "//tools/v8_context_snapshot:use_v8_context_snapshot",
     "//v8:external_startup_data",

gin/public/v8_platform.h

@@ -9,6 +9,7 @@
 #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 {
@@ -24,9 +25,14 @@ class GIN_EXPORT V8Platform : public v8::Platform {
 // v8::Platform implementation.
 // Some configurations do not use page_allocator.
 #if BUILDFLAG(USE_PARTITION_ALLOC)
-  v8::PageAllocator* GetPageAllocator() override;
+  // 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;
   void OnCriticalMemoryPressure() override;
 #endif
+
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
       v8::Isolate*) override;
   int NumberOfWorkerThreads() override;