Backed out 2 changesets (bug 1523526, bug 1526443) for Be bustage on Windows AArch on a CLOSED TREE
authorCoroiu Cristina <ccoroiu@mozilla.com>
Mon, 20 May 2019 20:21:56 +0300
changeset 474552 8fe96c8d21b7a9fdebdf3300de2367f2cc9232c3
parent 474551 31a636751e552086be71645df0439bcc128c0763
child 474553 0c1f4aee5de04bbef034b98f39c4e367b34cc5ba
push id36042
push userdvarga@mozilla.com
push dateTue, 21 May 2019 04:19:40 +0000
treeherdermozilla-central@ca560ff55451 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1523526, 1526443
milestone69.0a1
backs out98013639d60026eb3a0344fa499a321aec176d2b
e8ac4b512f9d03dae670be39b399ea8189e1ca05
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out 2 changesets (bug 1523526, bug 1526443) for Be bustage on Windows AArch on a CLOSED TREE Backed out changeset 98013639d600 (bug 1526443) Backed out changeset e8ac4b512f9d (bug 1523526)
browser/app/winlauncher/LauncherProcessWin.cpp
browser/config/mozconfigs/win64-aarch64/common-win64
build/build-clang/clang-win64.json
build/build-clang/r355141-arm64-cfg.patch
build/moz.configure/toolchain.configure
security/sandbox/chromium-shim/base/win/sdkdecls.h
security/sandbox/chromium-shim/patches/with_update/aarch64_control_flow_guard.patch
security/sandbox/chromium-shim/patches/with_update/patch_order.txt
security/sandbox/chromium/sandbox/win/src/process_mitigations.cc
security/sandbox/chromium/sandbox/win/src/security_level.h
security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
--- a/browser/app/winlauncher/LauncherProcessWin.cpp
+++ b/browser/app/winlauncher/LauncherProcessWin.cpp
@@ -55,45 +55,32 @@ static mozilla::LauncherVoidResult PostC
 }
 
 #if !defined( \
     PROCESS_CREATION_MITIGATION_POLICY_IMAGE_LOAD_PREFER_SYSTEM32_ALWAYS_ON)
 #  define PROCESS_CREATION_MITIGATION_POLICY_IMAGE_LOAD_PREFER_SYSTEM32_ALWAYS_ON \
     (0x00000001ULL << 60)
 #endif  // !defined(PROCESS_CREATION_MITIGATION_POLICY_IMAGE_LOAD_PREFER_SYSTEM32_ALWAYS_ON)
 
-#if !defined(PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF)
-#  define PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF \
-    (0x00000002ULL << 40)
-#endif  // !defined(PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF)
-
 #if (_WIN32_WINNT < 0x0602)
 BOOL WINAPI
 SetProcessMitigationPolicy(PROCESS_MITIGATION_POLICY aMitigationPolicy,
                            PVOID aBuffer, SIZE_T aBufferLen);
 #endif  // (_WIN32_WINNT >= 0x0602)
 
 /**
  * Any mitigation policies that should be set on the browser process should go
  * here.
  */
 static void SetMitigationPolicies(mozilla::ProcThreadAttributes& aAttrs,
                                   const bool aIsSafeMode) {
   if (mozilla::IsWin10AnniversaryUpdateOrLater()) {
     aAttrs.AddMitigationPolicy(
         PROCESS_CREATION_MITIGATION_POLICY_IMAGE_LOAD_PREFER_SYSTEM32_ALWAYS_ON);
   }
-
-#if defined(_M_ARM64)
-  // Disable CFG on older versions of ARM64 Windows to avoid a crash in COM.
-  if (!mozilla::IsWin10Sep2018UpdateOrLater()) {
-    aAttrs.AddMitigationPolicy(
-        PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF);
-  }
-#endif  // defined(_M_ARM64)
 }
 
 static mozilla::LauncherFlags ProcessCmdLine(int& aArgc, wchar_t* aArgv[]) {
   mozilla::LauncherFlags result = mozilla::LauncherFlags::eNone;
 
   if (mozilla::CheckArg(aArgc, aArgv, L"wait-for-browser",
                         static_cast<const wchar_t**>(nullptr),
                         mozilla::CheckArgFlag::RemoveArg) ==
--- a/browser/config/mozconfigs/win64-aarch64/common-win64
+++ b/browser/config/mozconfigs/win64-aarch64/common-win64
@@ -1,7 +1,3 @@
 # This file is used by all AArch64 Win64 builds
 
 ac_add_options --target=aarch64-windows-mingw32
-
-# Temporary signal to toolchain.configure that our compiler is patched to
-# support CFG, until such support can be assumed.
-ac_add_options --enable-hardening
--- a/build/build-clang/clang-win64.json
+++ b/build/build-clang/clang-win64.json
@@ -12,13 +12,12 @@
     "python_path": "c:/mozilla-build/python/python.exe",
     "cc": "cl.exe",
     "cxx": "cl.exe",
     "ml": "ml64.exe",
     "patches": [
       "workaround-issue38586.patch",
       "unpoison-thread-stacks.patch",
       "downgrade-mangling-error.patch",
-      "r355141-arm64-cfg.patch",
       "loosen-msvc-detection.patch",
       "revert-r355311.patch"
     ]
 }
deleted file mode 100644
--- a/build/build-clang/r355141-arm64-cfg.patch
+++ /dev/null
@@ -1,112 +0,0 @@
-[COFF] Add address-taken import thunks to the fid table
-
-https://bugs.llvm.org/show_bug.cgi?id=39799
-https://reviews.llvm.org/D58739
-
---- a/lld/COFF/Writer.cpp
-+++ b/lld/COFF/Writer.cpp
-@@ -1390,19 +1390,47 @@
- // symbol in an executable section.
- static void maybeAddAddressTakenFunction(SymbolRVASet &AddressTakenSyms,
-                                          Symbol *S) {
--  auto *D = dyn_cast_or_null<DefinedCOFF>(S);
--
--  // Ignore undefined symbols and references to non-functions (e.g. globals and
--  // labels).
--  if (!D ||
--      D->getCOFFSymbol().getComplexType() != COFF::IMAGE_SYM_DTYPE_FUNCTION)
-+  if (!S)
-     return;
- 
--  // Mark the symbol as address taken if it's in an executable section.
--  Chunk *RefChunk = D->getChunk();
--  OutputSection *OS = RefChunk ? RefChunk->getOutputSection() : nullptr;
--  if (OS && OS->Header.Characteristics & IMAGE_SCN_MEM_EXECUTE)
--    addSymbolToRVASet(AddressTakenSyms, D);
-+  switch (S->kind()) {
-+  case Symbol::DefinedLocalImportKind:
-+  case Symbol::DefinedImportDataKind:
-+    // Defines an __imp_ pointer, so it is data, so it is ignored.
-+    break;
-+  case Symbol::DefinedCommonKind:
-+    // Common is always data, so it is ignored.
-+    break;
-+  case Symbol::DefinedAbsoluteKind:
-+  case Symbol::DefinedSyntheticKind:
-+    // Absolute is never code, synthetic generally isn't and usually isn't
-+    // determinable.
-+    break;
-+  case Symbol::LazyKind:
-+  case Symbol::UndefinedKind:
-+    // Undefined symbols resolve to zero, so they don't have an RVA. Lazy
-+    // symbols shouldn't have relocations.
-+    break;
-+
-+  case Symbol::DefinedImportThunkKind:
-+    // Thunks are always code, include them.
-+    addSymbolToRVASet(AddressTakenSyms, cast<Defined>(S));
-+    break;
-+
-+  case Symbol::DefinedRegularKind: {
-+    // This is a regular, defined, symbol from a COFF file. Mark the symbol as
-+    // address taken if the symbol type is function and it's in an executable
-+    // section.
-+    auto *D = cast<DefinedRegular>(S);
-+    if (D->getCOFFSymbol().getComplexType() == COFF::IMAGE_SYM_DTYPE_FUNCTION) {
-+      Chunk *RefChunk = D->getChunk();
-+      OutputSection *OS = RefChunk ? RefChunk->getOutputSection() : nullptr;
-+      if (OS && OS->Header.Characteristics & IMAGE_SCN_MEM_EXECUTE)
-+        addSymbolToRVASet(AddressTakenSyms, D);
-+    }
-+    break;
-+  }
-+  }
- }
- 
- // Visit all relocations from all section contributions of this object file and
---- a/lld/test/COFF/guardcf-thunk.s
-+++ b/lld/test/COFF/guardcf-thunk.s
-@@ -0,0 +1,43 @@
-+# REQUIRES: x86
-+
-+# Make a DLL that exports exportfn1.
-+# RUN: yaml2obj < %p/Inputs/export.yaml > %t.obj
-+# RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1 /implib:%t.lib
-+
-+# Make an obj that takes the address of that exported function.
-+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc %s -o %t2.obj
-+# RUN: lld-link -entry:main -guard:cf %t2.obj %t.lib -nodefaultlib -out:%t.exe
-+# RUN: llvm-readobj -coff-load-config %t.exe | FileCheck %s
-+
-+# Check that the gfids table contains *exactly* two entries, one for exportfn1
-+# and one for main.
-+# CHECK: GuardFidTable [
-+# CHECK-NEXT: 0x{{[0-9A-Fa-f]+0$}}
-+# CHECK-NEXT: 0x{{[0-9A-Fa-f]+0$}}
-+# CHECK-NEXT: ]
-+
-+
-+        .def     @feat.00;
-+        .scl    3;
-+        .type   0;
-+        .endef
-+        .globl  @feat.00
-+@feat.00 = 0x001
-+
-+        .section .text,"rx"
-+        .def     main; .scl    2; .type   32; .endef
-+        .global main
-+main:
-+        leaq exportfn1(%rip), %rax
-+        retq
-+
-+        .section .rdata,"dr"
-+.globl _load_config_used
-+_load_config_used:
-+        .long 256
-+        .fill 124, 1, 0
-+        .quad __guard_fids_table
-+        .quad __guard_fids_count
-+        .long __guard_flags
-+        .fill 128, 1, 0
-+
--- a/build/moz.configure/toolchain.configure
+++ b/build/moz.configure/toolchain.configure
@@ -1661,20 +1661,19 @@ def security_hardening_cflags(hardening_
         # ASLR ------------------------------------------------
         # ASLR (dynamicbase) is enabled by default in clang-cl; but the
         # mingw-clang build requires it to be explicitly enabled
         if target.os == 'WINNT' and c_compiler.type == 'clang':
             ldflags.append("-Wl,--dynamicbase")
             js_ldflags.append("-Wl,--dynamicbase")
 
         # Control Flow Guard (CFG) ----------------------------
-        # On aarch64, this is enabled only with explicit --enable-hardening
-        # (roughly: automation) due to a dependency on a patched clang-cl.
+        # See Bug 1525588 for why this doesn't work on Windows ARM
         if c_compiler.type == 'clang-cl' and c_compiler.version >= '8' and \
-           (target.cpu != 'aarch64' or hardening_flag):
+           target.cpu != 'aarch64':
             flags.append("-guard:cf")
             js_flags.append("-guard:cf")
             # nolongjmp is needed because clang doesn't emit the CFG tables of
             # setjmp return addresses https://bugs.llvm.org/show_bug.cgi?id=40057
             ldflags.append("-guard:cf,nolongjmp")
             js_ldflags.append("-guard:cf,nolongjmp")
 
     # ----------------------------------------------------------
--- a/security/sandbox/chromium-shim/base/win/sdkdecls.h
+++ b/security/sandbox/chromium-shim/base/win/sdkdecls.h
@@ -206,27 +206,16 @@ typedef struct _PROCESS_MITIGATION_EXTEN
 
 #define PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_MASK                     (0x00000003uLL << 36)
 #define PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_DEFER                    (0x00000000uLL << 36)
 #define PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_ALWAYS_ON                (0x00000001uLL << 36)
 #define PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_ALWAYS_OFF               (0x00000002uLL << 36)
 #define PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_ALWAYS_ON_ALLOW_OPT_OUT  (0x00000003uLL << 36)
 
 //
-// Define Control Flow Guard (CFG) mitigation policy options.  Control Flow
-// Guard allows indirect control transfers to be checked at runtime.
-//
-
-#define PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_MASK                        (0x00000003uLL << 40)
-#define PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_DEFER                       (0x00000000uLL << 40)
-#define PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_ON                   (0x00000001uLL << 40)
-#define PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF                  (0x00000002uLL << 40)
-#define PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_EXPORT_SUPPRESSION          (0x00000003uLL << 40)
-
-//
 // Define module signature options.  When enabled, this option will
 // block mapping of non-microsoft binaries.
 //
 
 #define PROCESS_CREATION_MITIGATION_POLICY_BLOCK_NON_MICROSOFT_BINARIES_MASK              (0x00000003uLL << 44)
 #define PROCESS_CREATION_MITIGATION_POLICY_BLOCK_NON_MICROSOFT_BINARIES_DEFER             (0x00000000uLL << 44)
 #define PROCESS_CREATION_MITIGATION_POLICY_BLOCK_NON_MICROSOFT_BINARIES_ALWAYS_ON         (0x00000001uLL << 44)
 #define PROCESS_CREATION_MITIGATION_POLICY_BLOCK_NON_MICROSOFT_BINARIES_ALWAYS_OFF        (0x00000002uLL << 44)
deleted file mode 100644
--- a/security/sandbox/chromium-shim/patches/with_update/aarch64_control_flow_guard.patch
+++ /dev/null
@@ -1,44 +0,0 @@
-Bug 1523526: Don't allow CFG on old releases of Windows for arm64
-
-There's a bug in ole32.dll on arm64 versions of Windows prior to 1809, that crashes our content processes if we enable CFG. We've reported the issue, but even if it gets fixed, we can't assume users will have the update.
-
-This patch uses process mitigation policy flags to disable CFG on arm64 before 1809. Based on testing, we only need to do this in the sandbox for child processes, and it's not strictly necessary for the launcher stub to set the flag on the main process. But I've included that anyway as a guard against some yet-undiscovered scenario that might hit the issue and make the browser unusable.
-
-The effects of this patch won't be visible until we actually enable CFG in a subsequent landing.
-
-Differential Revision: https://phabricator.services.mozilla.com/D29474
-
-diff --git a/security/sandbox/chromium/sandbox/win/src/process_mitigations.cc b/security/sandbox/chromium/sandbox/win/src/process_mitigations.cc
---- a/security/sandbox/chromium/sandbox/win/src/process_mitigations.cc
-+++ b/security/sandbox/chromium/sandbox/win/src/process_mitigations.cc
-@@ -354,6 +354,11 @@ void ConvertProcessMitigationsToPolicy(M
-         PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_ALWAYS_ON;
-   }
- 
-+  if (flags & MITIGATION_CONTROL_FLOW_GUARD_DISABLE) {
-+    *policy_flags |=
-+        PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF;
-+  }
-+
-   if (version < base::win::VERSION_WIN10)
-     return;
- 
-diff --git a/security/sandbox/chromium/sandbox/win/src/security_level.h b/security/sandbox/chromium/sandbox/win/src/security_level.h
---- a/security/sandbox/chromium/sandbox/win/src/security_level.h
-+++ b/security/sandbox/chromium/sandbox/win/src/security_level.h
-@@ -255,6 +255,15 @@ const MitigationFlags MITIGATION_IMAGE_L
- // PROCESS_CREATION_MITIGATION_POLICY_IMAGE_LOAD_PREFER_SYSTEM32_ALWAYS_ON.
- const MitigationFlags MITIGATION_IMAGE_LOAD_PREFER_SYS32 = 0x00100000;
- 
-+// Begin Mozilla-added flags.
-+// Working down from the high bit to avoid conflict with new upstream flags.
-+
-+// Disable Control Flow Guard. This may seem more like an anti-mitigation, but
-+// this flag allows code to make targeted changes to CFG to avoid bugs, while
-+// leaving it enabled in the common case. Corresponds to
-+// PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_ON.
-+const MitigationFlags MITIGATION_CONTROL_FLOW_GUARD_DISABLE = 0x80000000;
-+
- }  // namespace sandbox
- 
- #endif  // SANDBOX_SRC_SECURITY_LEVEL_H_
--- a/security/sandbox/chromium-shim/patches/with_update/patch_order.txt
+++ b/security/sandbox/chromium-shim/patches/with_update/patch_order.txt
@@ -16,9 +16,8 @@ mingw_duplicate_instatinations.patch
 mingw_copy_s.patch
 mingw_operator_new.patch
 mingw_cast_getprocaddress.patch
 mingw_capitalization.patch
 mingw_disable_one_try.patch
 mingw_noexports_casts.patch
 mingw_offsetof.patch
 add_aarch64_windows_support.patch
-aarch64_control_flow_guard.patch
--- a/security/sandbox/chromium/sandbox/win/src/process_mitigations.cc
+++ b/security/sandbox/chromium/sandbox/win/src/process_mitigations.cc
@@ -349,21 +349,16 @@ void ConvertProcessMitigationsToPolicy(M
   if (version < base::win::VERSION_WIN8_1)
     return;
 
   if (flags & MITIGATION_DYNAMIC_CODE_DISABLE) {
     *policy_flags |=
         PROCESS_CREATION_MITIGATION_POLICY_PROHIBIT_DYNAMIC_CODE_ALWAYS_ON;
   }
 
-  if (flags & MITIGATION_CONTROL_FLOW_GUARD_DISABLE) {
-    *policy_flags |=
-        PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_OFF;
-  }
-
   if (version < base::win::VERSION_WIN10)
     return;
 
   if (flags & MITIGATION_NONSYSTEM_FONT_DISABLE) {
     *policy_flags |= PROCESS_CREATION_MITIGATION_POLICY_FONT_DISABLE_ALWAYS_ON;
   }
 
   // Threshold 2
--- a/security/sandbox/chromium/sandbox/win/src/security_level.h
+++ b/security/sandbox/chromium/sandbox/win/src/security_level.h
@@ -250,20 +250,11 @@ const MitigationFlags MITIGATION_IMAGE_L
 
 // Forces image load preference to prioritize the Windows install System32
 // folder before dll load dir, application dir and any user dirs set.
 // - Affects IAT resolution standard search path only, NOT direct LoadLibrary or
 //   executable search path.
 // PROCESS_CREATION_MITIGATION_POLICY_IMAGE_LOAD_PREFER_SYSTEM32_ALWAYS_ON.
 const MitigationFlags MITIGATION_IMAGE_LOAD_PREFER_SYS32 = 0x00100000;
 
-// Begin Mozilla-added flags.
-// Working down from the high bit to avoid conflict with new upstream flags.
-
-// Disable Control Flow Guard. This may seem more like an anti-mitigation, but
-// this flag allows code to make targeted changes to CFG to avoid bugs, while
-// leaving it enabled in the common case. Corresponds to
-// PROCESS_CREATION_MITIGATION_POLICY_CONTROL_FLOW_GUARD_ALWAYS_ON.
-const MitigationFlags MITIGATION_CONTROL_FLOW_GUARD_DISABLE = 0x80000000;
-
 }  // namespace sandbox
 
 #endif  // SANDBOX_SRC_SECURITY_LEVEL_H_
--- a/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
+++ b/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ -503,23 +503,16 @@ void SandboxBroker::SetSecurityLevelForC
       "SetDelayedIntegrityLevel should never fail, what happened?");
 
   sandbox::MitigationFlags mitigations =
       sandbox::MITIGATION_BOTTOM_UP_ASLR | sandbox::MITIGATION_HEAP_TERMINATE |
       sandbox::MITIGATION_SEHOP | sandbox::MITIGATION_DEP_NO_ATL_THUNK |
       sandbox::MITIGATION_DEP | sandbox::MITIGATION_EXTENSION_POINT_DISABLE |
       sandbox::MITIGATION_IMAGE_LOAD_PREFER_SYS32;
 
-#if defined(_M_ARM64)
-  // Disable CFG on older versions of ARM64 Windows to avoid a crash in COM.
-  if (!IsWin10Sep2018UpdateOrLater()) {
-    mitigations |= sandbox::MITIGATION_CONTROL_FLOW_GUARD_DISABLE;
-  }
-#endif
-
   if (aSandboxLevel > 4) {
     result = mPolicy->SetAlternateDesktop(false);
     if (NS_WARN_IF(result != sandbox::SBOX_ALL_OK)) {
       LOG_W("SetAlternateDesktop failed, result: %i, last error: %x", result,
             ::GetLastError());
     }
   }