[wasm] Initialize memory protection key permissions

Initialize the (thread-local) memory protection key permissions for any
isolate that joins the wasm engine. Otherwise it can happen that an
isolate gets Wasm code from the cache without ever compiling anything
(hence without ever changing memory protection key permissions), and
then it would not be allowed to access (read or execute) the code.

I tested this change manually on a PKU-enabled devices. The new test
crashed before the fix, and completes successfully afterwards.

R=ahaas@chromium.org

Bug: v8:11974, chromium:1280451
Change-Id: I90dded8b4fdaa8cf34b44107291d3f525ce16335
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3347563
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78413}

src/wasm/memory-protection-key.cc

@@ -184,7 +184,7 @@ void SetPermissionsForMemoryProtectionKey(
 }
 
 DISABLE_CFI_ICALL
-bool MemoryProtectionKeyWritable(int key) {
+MemoryProtectionKeyPermission GetMemoryProtectionKeyPermission(int key) {
   DCHECK_NE(kNoMemoryProtectionKey, key);
 
 #if defined(V8_OS_LINUX) && defined(V8_HOST_ARCH_X64)
@@ -193,8 +193,10 @@ bool MemoryProtectionKeyWritable(int key) {
   // If a valid key was allocated, {pkey_get()} must also be available.
   DCHECK_NOT_NULL(pkey_get);
 
-  int permissions = pkey_get(key);
-  return permissions == kNoRestrictions;
+  int permission = pkey_get(key);
+  CHECK(permission == kNoRestrictions || permission == kDisableAccess ||
+        permission == kDisableWrite);
+  return static_cast<MemoryProtectionKeyPermission>(permission);
 #else
   // On platforms without PKU support, this method cannot be called because
   // no protection key can have been allocated.

src/wasm/memory-protection-key.h

@@ -82,9 +82,8 @@ bool SetPermissionsAndMemoryProtectionKey(
 void SetPermissionsForMemoryProtectionKey(
     int key, MemoryProtectionKeyPermission permissions);
 
-// Returns {true} if the protection key {key} is write-enabled for the current
-// thread.
-bool MemoryProtectionKeyWritable(int key);
+// Get the permissions of the protection key {key} for the current thread.
+MemoryProtectionKeyPermission GetMemoryProtectionKeyPermission(int key);
 
 }  // namespace wasm
 }  // namespace internal

src/wasm/wasm-code-manager.cc

@@ -2124,7 +2124,20 @@ bool WasmCodeManager::MemoryProtectionKeysEnabled() const {
 }
 
 bool WasmCodeManager::MemoryProtectionKeyWritable() const {
-  return wasm::MemoryProtectionKeyWritable(memory_protection_key_);
+  return GetMemoryProtectionKeyPermission(memory_protection_key_) ==
+         MemoryProtectionKeyPermission::kNoRestrictions;
+}
+
+void WasmCodeManager::InitializeMemoryProtectionKeyPermissionsIfSupported()
+    const {
+  if (!HasMemoryProtectionKeySupport()) return;
+  // The default permission is {kDisableAccess}. Switch from that to
+  // {kDisableWrite}. Leave other permissions untouched, as the thread did
+  // already use the memory protection key in that case.
+  if (GetMemoryProtectionKeyPermission(memory_protection_key_) ==
+      kDisableAccess) {
+    SetPermissionsForMemoryProtectionKey(memory_protection_key_, kDisableWrite);
+  }
 }
 
 std::shared_ptr<NativeModule> WasmCodeManager::NewNativeModule(

src/wasm/wasm-code-manager.h

@@ -1058,6 +1058,10 @@ class V8_EXPORT_PRIVATE WasmCodeManager final {
   // Can only be called if {HasMemoryProtectionKeySupport()} is {true}.
   bool MemoryProtectionKeyWritable() const;
 
+  // Initialize the current thread's permissions for the memory protection key,
+  // if we have support.
+  void InitializeMemoryProtectionKeyPermissionsIfSupported() const;
+
  private:
   friend class WasmCodeAllocator;
   friend class WasmEngine;

src/wasm/wasm-engine.cc

@@ -1017,6 +1017,12 @@ void WasmEngine::AddIsolate(Isolate* isolate) {
   DCHECK_EQ(0, isolates_.count(isolate));
   isolates_.emplace(isolate, std::make_unique<IsolateInfo>(isolate));
 
+  // The isolate might access existing (cached) code without ever compiling any.
+  // In that case, the current thread might still have the default permissions
+  // for the memory protection key (== no access). Thus initialize the
+  // permissions now.
+  GetWasmCodeManager()->InitializeMemoryProtectionKeyPermissionsIfSupported();
+
   // Install sampling GC callback.
   // TODO(v8:7424): For now we sample module sizes in a GC callback. This will
   // bias samples towards apps with high memory pressure. We should switch to

test/cctest/wasm/test-compilation-cache.cc

@@ -233,6 +233,84 @@ TEST(TestStreamingAndSyncCache) {
   CHECK_EQ(native_module_streaming, native_module_sync);
 }
 
+void TestModuleSharingBetweenIsolates() {
+  class ShareModuleThread : public base::Thread {
+   public:
+    ShareModuleThread(
+        const char* name,
+        std::function<void(std::shared_ptr<NativeModule>)> register_module)
+        : base::Thread(base::Thread::Options{name}),
+          register_module_(std::move(register_module)) {}
+
+    void Run() override {
+      v8::Isolate::CreateParams isolate_create_params;
+      auto* ab_allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator();
+      isolate_create_params.array_buffer_allocator = ab_allocator;
+      v8::Isolate* isolate = v8::Isolate::New(isolate_create_params);
+      Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate);
+      isolate->Enter();
+
+      {
+        i::HandleScope handle_scope(i_isolate);
+        v8::Context::New(isolate)->Enter();
+        auto full_bytes =
+            base::OwnedVector<uint8_t>::New(kPrefixSize + kFunctionSize);
+        memcpy(full_bytes.begin(), kPrefix, kPrefixSize);
+        memcpy(full_bytes.begin() + kPrefixSize, kFunctionA, kFunctionSize);
+        ErrorThrower thrower(i_isolate, "Test");
+        std::shared_ptr<NativeModule> native_module =
+            GetWasmEngine()
+                ->SyncCompile(i_isolate, WasmFeatures::All(), &thrower,
+                              ModuleWireBytes{full_bytes.as_vector()})
+                .ToHandleChecked()
+                ->shared_native_module();
+        register_module_(native_module);
+        // Check that we can access the code (see https://crbug.com/1280451).
+        WasmCodeRefScope code_ref_scope;
+        uint8_t* code_start = native_module->GetCode(0)->instructions().begin();
+        // Use the loaded value in a CHECK to prevent the compiler from just
+        // optimizing it away. Even {volatile} would require that.
+        CHECK_NE(0, *code_start);
+      }
+
+      isolate->Exit();
+      isolate->Dispose();
+      delete ab_allocator;
+    }
+
+   private:
+    const std::function<void(std::shared_ptr<NativeModule>)> register_module_;
+  };
+
+  std::vector<std::shared_ptr<NativeModule>> modules;
+  base::Mutex mutex;
+  auto register_module = [&](std::shared_ptr<NativeModule> module) {
+    base::MutexGuard guard(&mutex);
+    modules.emplace_back(std::move(module));
+  };
+
+  ShareModuleThread thread1("ShareModuleThread1", register_module);
+  CHECK(thread1.Start());
+  thread1.Join();
+
+  // Start a second thread which should get the cached module.
+  ShareModuleThread thread2("ShareModuleThread2", register_module);
+  CHECK(thread2.Start());
+  thread2.Join();
+
+  CHECK_EQ(2, modules.size());
+  CHECK_EQ(modules[0].get(), modules[1].get());
+}
+
+UNINITIALIZED_TEST(TwoIsolatesShareNativeModule) {
+  TestModuleSharingBetweenIsolates();
+}
+
+UNINITIALIZED_TEST(TwoIsolatesShareNativeModuleWithPku) {
+  FLAG_wasm_memory_protection_keys = true;
+  TestModuleSharingBetweenIsolates();
+}
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8