[wasm] Reset PKRU before spawning new threads

We sometimes hit the DCHECK in the wasm code manager:
  DCHECK_IMPLIES(writable, !MemoryProtectionKeyWritable());

This is because we spawn new threads while having a
{CodeSpaceWriteScope} open. In the case of PKU, this changes the PKRU
register to allow writes to the code space, and the value of that
register is inherited by any new thread. If this thread then tries to
switch to writable code spaces, it hits the DCHECK. It would hit a
similar DCHECK when trying to execute code.

We fix this issue by temporarily resetting the PKRU register to
non-writable while we call the {NotifyConcurrencyIncrease} method. This
is not a very robust solution, as any new call that potentially happens
inside a {CodeSpaceWriteScope} needs to do the same, but refactoring the
code to avoid spawning new threads while being in writable state would
be a lot of work with other downsides.

R=jkummerow@chromium.org

Bug: v8:13075
Change-Id: Ibc7270aa597902dc6d9649cb6bcdfce8b1a9bafc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3762579
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81729}

src/wasm/code-space-access.cc

@@ -96,6 +96,17 @@ bool CodeSpaceWriteScope::SwitchingPerNativeModule() {
          FLAG_wasm_write_protect_code_memory;
 }
 
+ResetPKUPermissionsForThreadSpawning::ResetPKUPermissionsForThreadSpawning() {
+  auto* code_manager = GetWasmCodeManager();
+  was_writable_ = code_manager->MemoryProtectionKeysEnabled() &&
+                  code_manager->MemoryProtectionKeyWritable();
+  if (was_writable_) code_manager->SetThreadWritable(false);
+}
+
+ResetPKUPermissionsForThreadSpawning::~ResetPKUPermissionsForThreadSpawning() {
+  if (was_writable_) GetWasmCodeManager()->SetThreadWritable(true);
+}
+
 #endif  // !V8_HAS_PTHREAD_JIT_WRITE_PROTECT
 
 }  // namespace wasm

src/wasm/code-space-access.h

@@ -12,10 +12,7 @@
 #include "src/base/build_config.h"
 #include "src/base/macros.h"
 
-namespace v8 {
-namespace internal {
-
-namespace wasm {
+namespace v8::internal::wasm {
 
 class NativeModule;
 
@@ -75,8 +72,24 @@ class V8_NODISCARD CodeSpaceWriteScope final {
   NativeModule* const previous_native_module_;
 };
 
-}  // namespace wasm
-}  // namespace internal
-}  // namespace v8
+// Sometimes we need to call a function which will / might spawn a new thread,
+// like {JobHandle::NotifyConcurrencyIncrease}, while holding a
+// {CodeSpaceWriteScope}. This is problematic since the new thread will inherit
+// the parent thread's PKU permissions.
+// The {ResetPKUPermissionsForThreadSpawning} scope will thus reset the PKU
+// permissions as long as it is in scope, such that it is safe to spawn new
+// threads.
+class V8_NODISCARD ResetPKUPermissionsForThreadSpawning {
+#if !V8_HAS_PTHREAD_JIT_WRITE_PROTECT
+ public:
+  ResetPKUPermissionsForThreadSpawning();
+  ~ResetPKUPermissionsForThreadSpawning();
+
+ private:
+  bool was_writable_;
+#endif
+};
+
+}  // namespace v8::internal::wasm
 
 #endif  // V8_WASM_CODE_SPACE_ACCESS_H_

src/wasm/module-compiler.cc

@@ -3404,6 +3404,7 @@ void CompilationStateImpl::CommitCompilationUnits(
     compilation_unit_queues_.AddUnits(baseline_units, top_tier_units,
                                       native_module_->module());
   }
+  ResetPKUPermissionsForThreadSpawning pku_reset_scope;
   compile_job_->NotifyConcurrencyIncrease();
 }
 
@@ -3415,6 +3416,10 @@ void CompilationStateImpl::CommitTopTierCompilationUnit(
 void CompilationStateImpl::AddTopTierPriorityCompilationUnit(
     WasmCompilationUnit unit, size_t priority) {
   compilation_unit_queues_.AddTopTierPriorityUnit(unit, priority);
+  // We should not have a {CodeSpaceWriteScope} open at this point, as
+  // {NotifyConcurrencyIncrease} can spawn new threads which could inherit PKU
+  // permissions (which would be a security issue).
+  DCHECK(!CodeSpaceWriteScope::IsInScope());
   compile_job_->NotifyConcurrencyIncrease();
 }
 

src/wasm/wasm-serialization.cc

@@ -596,6 +596,7 @@ class DeserializeCodeTask : public JobTask {
         deserializer_->CopyAndRelocate(unit);
       }
       publish_queue_.Add(std::move(batch));
+      ResetPKUPermissionsForThreadSpawning pku_reset_scope;
       delegate->NotifyConcurrencyIncrease();
     }
   }
@@ -678,6 +679,7 @@ bool NativeModuleDeserializer::Read(Reader* reader) {
       reloc_queue.Add(std::move(batch));
       DCHECK(batch.empty());
       batch_size = 0;
+      ResetPKUPermissionsForThreadSpawning pku_reset_scope;
       job_handle->NotifyConcurrencyIncrease();
     }
   }
@@ -689,6 +691,7 @@ bool NativeModuleDeserializer::Read(Reader* reader) {
 
   if (!batch.empty()) {
     reloc_queue.Add(std::move(batch));
+    ResetPKUPermissionsForThreadSpawning pku_reset_scope;
     job_handle->NotifyConcurrencyIncrease();
   }