From 302ec1c38d4774fa69566517f5be79896f39bae5 Mon Sep 17 00:00:00 2001 From: Jan de Mooij Date: Wed, 7 Jun 2023 16:34:51 +0000 Subject: [PATCH] Bug 1835876 part 2 - Disable code write protection in content processes. r=nbp We've worked with the security and performance teams to re-evaluate the W^X policy we have in place to mark JIT code memory pages as either writable or executable (but not both). Recommendation from the security team is to disable this mitigation in the content process, because it's not worth the performance cost since there are known and reusable techniques to bypass it. The V8 team has recently reached a similar conclusion. We still leave write protection enabled in the parent process and other non-content processes, because performance is less of a concern there and it's unclear if the techniques to bypass this mitigation apply to these processes as well. This patch adds a `javascript.options.content_process_write_protect_code` pref for this and sets it to false. The JS shell has a new `--write-protect-code=off/on` flag. This defaults to `on` for now to help catch W^X violations early on. This is expected to improve performance on Speedometer 3 by about 3-4%. We've seen similar numbers for other benchmarks including page load measurements. Depends on D179468 Differential Revision: https://phabricator.services.mozilla.com/D179469 --- js/src/jit/JitOptions.cpp | 14 ++++++++++++++ js/src/jit/JitOptions.h | 4 ++++ js/src/jit/ProcessExecutableMemory.cpp | 10 ++++++++++ js/src/jsapi.cpp | 6 ++++++ js/src/jsapi.h | 1 + js/src/shell/fuzz-flags.txt | 2 ++ js/src/shell/js.cpp | 15 +++++++++++++++ js/src/tests/lib/tests.py | 8 ++++---- js/xpconnect/src/XPCJSContext.cpp | 8 ++++++++ modules/libpref/init/StaticPrefList.yaml | 7 +++++++ 10 files changed, 71 insertions(+), 4 deletions(-) diff --git a/js/src/jit/JitOptions.cpp b/js/src/jit/JitOptions.cpp index 1dc24f1ce2d5..978661303692 100644 --- a/js/src/jit/JitOptions.cpp +++ b/js/src/jit/JitOptions.cpp @@ -280,6 +280,10 @@ DefaultJitOptions::DefaultJitOptions() { SET_DEFAULT(spectreJitToCxxCalls, true); #endif + // Whether the W^X policy is enforced to mark JIT code pages as either + // writable or executable but never both at the same time. + SET_DEFAULT(writeProtectCode, true); + // This is set to its actual value in InitializeJit. SET_DEFAULT(supportsUnalignedAccesses, false); @@ -410,5 +414,15 @@ void DefaultJitOptions::resetNormalIonWarmUpThreshold() { setNormalIonWarmUpThreshold(defaultValues.normalIonWarmUpThreshold); } +void DefaultJitOptions::maybeSetWriteProtectCode(bool val) { + // For now don't change the default (true) on Apple ARM64 because we can't use + // RWX pages there without MAP_JIT. See bug 1837194. +#if defined(XP_MACOSX) && defined(__aarch64__) + MOZ_ASSERT(writeProtectCode); +#else + writeProtectCode = val; +#endif +} + } // namespace jit } // namespace js diff --git a/js/src/jit/JitOptions.h b/js/src/jit/JitOptions.h index b0599b012f55..12328891bf0d 100644 --- a/js/src/jit/JitOptions.h +++ b/js/src/jit/JitOptions.h @@ -122,6 +122,8 @@ struct DefaultJitOptions { bool spectreValueMasking; bool spectreJitToCxxCalls; + bool writeProtectCode; + bool supportsUnalignedAccesses; BaseRegForAddress baseRegForLocals; @@ -146,6 +148,8 @@ struct DefaultJitOptions { void enableGvn(bool val); void setFastWarmUp(); + void maybeSetWriteProtectCode(bool val); + bool eagerIonCompilation() const { return normalIonWarmUpThreshold == 0; } }; diff --git a/js/src/jit/ProcessExecutableMemory.cpp b/js/src/jit/ProcessExecutableMemory.cpp index f4470f54e4b4..870dab500930 100644 --- a/js/src/jit/ProcessExecutableMemory.cpp +++ b/js/src/jit/ProcessExecutableMemory.cpp @@ -384,6 +384,9 @@ static void DeallocateProcessExecutableMemory(void* addr, size_t bytes) { } static DWORD ProtectionSettingToFlags(ProtectionSetting protection) { + if (!JitOptions.writeProtectCode) { + return PAGE_EXECUTE_READWRITE; + } switch (protection) { case ProtectionSetting::Writable: return PAGE_READWRITE; @@ -496,6 +499,9 @@ static void DeallocateProcessExecutableMemory(void* addr, size_t bytes) { } static unsigned ProtectionSettingToFlags(ProtectionSetting protection) { + if (!JitOptions.writeProtectCode) { + return PROT_READ | PROT_WRITE | PROT_EXEC; + } # ifdef MOZ_VALGRIND // If we're configured for Valgrind and running on it, use a slacker // scheme that doesn't change execute permissions, since doing so causes @@ -890,6 +896,10 @@ bool js::jit::ReprotectRegion(void* start, size_t size, #else std::atomic_thread_fence(std::memory_order_seq_cst); + if (!JitOptions.writeProtectCode) { + return true; + } + # ifdef XP_WIN DWORD flags = ProtectionSettingToFlags(protection); // This is a essentially a VirtualProtect, but with lighter impact on diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 47008cb55d95..e10b33791fb3 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -4225,6 +4225,9 @@ JS_PUBLIC_API void JS_SetGlobalJitCompilerOption(JSContext* cx, case JSJITCOMPILER_SPECTRE_JIT_TO_CXX_CALLS: jit::JitOptions.spectreJitToCxxCalls = !!value; break; + case JSJITCOMPILER_WRITE_PROTECT_CODE: + jit::JitOptions.maybeSetWriteProtectCode(!!value); + break; case JSJITCOMPILER_WATCHTOWER_MEGAMORPHIC: jit::JitOptions.enableWatchtowerMegamorphic = !!value; break; @@ -4314,6 +4317,9 @@ JS_PUBLIC_API bool JS_GetGlobalJitCompilerOption(JSContext* cx, case JSJITCOMPILER_SPECTRE_JIT_TO_CXX_CALLS: *valueOut = jit::JitOptions.spectreJitToCxxCalls ? 1 : 0; break; + case JSJITCOMPILER_WRITE_PROTECT_CODE: + *valueOut = jit::JitOptions.writeProtectCode ? 1 : 0; + break; case JSJITCOMPILER_WATCHTOWER_MEGAMORPHIC: *valueOut = jit::JitOptions.enableWatchtowerMegamorphic ? 1 : 0; break; diff --git a/js/src/jsapi.h b/js/src/jsapi.h index fe7a4d84e7db..e914dc7be56d 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -836,6 +836,7 @@ extern JS_PUBLIC_API void JS_SetOffthreadIonCompilationEnabled(JSContext* cx, Register(SPECTRE_STRING_MITIGATIONS, "spectre.string-mitigations") \ Register(SPECTRE_VALUE_MASKING, "spectre.value-masking") \ Register(SPECTRE_JIT_TO_CXX_CALLS, "spectre.jit-to-cxx-calls") \ + Register(WRITE_PROTECT_CODE, "write-protect-code") \ Register(WATCHTOWER_MEGAMORPHIC, "watchtower.megamorphic") \ Register(WASM_FOLD_OFFSETS, "wasm.fold-offsets") \ Register(WASM_DELAY_TIER2, "wasm.delay-tier2") \ diff --git a/js/src/shell/fuzz-flags.txt b/js/src/shell/fuzz-flags.txt index 0c16988fc687..4df4310906fa 100644 --- a/js/src/shell/fuzz-flags.txt +++ b/js/src/shell/fuzz-flags.txt @@ -50,6 +50,8 @@ --nursery-bigints=on --spectre-mitigations=off --spectre-mitigations=on +--write-protect-code=off +--write-protect-code=on --more-compartments --fast-warmup --no-jit-backend diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index a49d35da3b72..7c49c5e12d9f 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -11437,6 +11437,11 @@ bool InitOptionParser(OptionParser& op) { !op.addStringOption('\0', "spectre-mitigations", "on/off", "Whether Spectre mitigations are enabled (default: " "off, on to enable)") || + !op.addStringOption('\0', "write-protect-code", "on/off", + "Whether the W^X policy is enforced to mark JIT code " + "pages as either writable or executable but never " + "both at the same time (default: on, off to " + "disable)") || !op.addStringOption('\0', "cache-ir-stubs", "on/off/call", "Use CacheIR stubs (default: on, off to disable, " "call to enable work-in-progress call ICs)") || @@ -12156,6 +12161,16 @@ bool SetContextJITOptions(JSContext* cx, const OptionParser& op) { } } + if (const char* str = op.getStringOption("write-protect-code")) { + if (strcmp(str, "on") == 0) { + jit::JitOptions.maybeSetWriteProtectCode(true); + } else if (strcmp(str, "off") == 0) { + jit::JitOptions.maybeSetWriteProtectCode(false); + } else { + return OptionFailure("write-protect-code", str); + } + } + if (const char* str = op.getStringOption("ion-scalar-replacement")) { if (strcmp(str, "on") == 0) { jit::JitOptions.disableScalarReplacement = false; diff --git a/js/src/tests/lib/tests.py b/js/src/tests/lib/tests.py index bc86b24d5cbe..574b48b642eb 100644 --- a/js/src/tests/lib/tests.py +++ b/js/src/tests/lib/tests.py @@ -25,7 +25,7 @@ JITFLAGS = { "--no-sse3", "--no-threads", ], - ["--baseline-eager"], + ["--baseline-eager", "--write-protect-code=off"], ["--no-blinterp", "--no-baseline", "--no-ion", "--more-compartments"], ["--blinterp-eager"], ], @@ -38,12 +38,12 @@ JITFLAGS = { "--ion-offthread-compile=off", # implies --baseline-eager "--more-compartments", ], - ["--baseline-eager"], + ["--baseline-eager", "--write-protect-code=off"], ["--no-blinterp", "--no-baseline", "--no-ion", "--more-compartments"], ], # used by jit_test.py "ion": [ - ["--baseline-eager"], + ["--baseline-eager", "--write-protect-code=off"], ["--ion-eager", "--ion-offthread-compile=off", "--more-compartments"], ], # Run reduced variants on debug builds, since they take longer time. @@ -54,7 +54,7 @@ JITFLAGS = { "--ion-offthread-compile=off", # implies --baseline-eager "--more-compartments", ], - ["--baseline-eager"], + ["--baseline-eager", "--write-protect-code=off"], ], # Cover cases useful for tsan. Note that we test --ion-eager without # --ion-offthread-compile=off here, because it helps catch races. diff --git a/js/xpconnect/src/XPCJSContext.cpp b/js/xpconnect/src/XPCJSContext.cpp index 0f54e00cd592..f71e71cfcbb6 100644 --- a/js/xpconnect/src/XPCJSContext.cpp +++ b/js/xpconnect/src/XPCJSContext.cpp @@ -969,6 +969,14 @@ static void LoadStartupJSPrefs(XPCJSContext* xpccx) { javascript_options_spectre_jit_to_cxx_calls_DoNotUseDirectly()); #endif + bool writeProtectCode = true; + if (XRE_IsContentProcess()) { + writeProtectCode = + StaticPrefs::javascript_options_content_process_write_protect_code(); + } + JS_SetGlobalJitCompilerOption(cx, JSJITCOMPILER_WRITE_PROTECT_CODE, + writeProtectCode); + JS_SetGlobalJitCompilerOption( cx, JSJITCOMPILER_WATCHTOWER_MEGAMORPHIC, StaticPrefs:: diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 3a27df1e3923..e676c06acc46 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -7829,6 +7829,13 @@ value: false mirror: always +# Whether the W^X policy is enforced to mark JIT code pages as either writable +# or executable but never both at the same time. +- name: javascript.options.content_process_write_protect_code + type: bool + value: false + mirror: always + # Whether to use the XPCOM thread pool for JS helper tasks. - name: javascript.options.external_thread_pool type: bool