Fix dynamic code loading operations

src/include/concurrency_ops.h

@@ -55,28 +55,26 @@ static INLINE void NaClWriteMemoryBarrier(void) {
 #endif
 
 
+/*
+ * If it needs to work on Rosetta, RosettaFlushInstructionCache is additionally
+ * needed.
+ */
 static INLINE void NaClFlushCacheForDoublyMappedCode(uint8_t *writable_addr,
                                                      uint8_t *executable_addr,
                                                      size_t size) {
-#if NACL_ARCH(NACL_BUILD_ARCH) == NACL_x86
+#if NACL_WINDOWS
   /*
-   * Clearing the icache explicitly is not necessary on x86.  We could
-   * call gcc's __builtin___clear_cache() on x86, where it is a no-op,
-   * except that it is not available in Mac OS X's old version of gcc.
-   * We simply prevent the compiler from moving loads or stores around
+   * Clearing the icache explicitly is not necessary on x86.  The compiler
+   * only must be prevented from moving loads or stores around
    * this function.
    */
   NACL_UNUSED_PARAMETER(writable_addr);
   NACL_UNUSED_PARAMETER(executable_addr);
   NACL_UNUSED_PARAMETER(size);
-#if NACL_WINDOWS
   _ReadWriteBarrier();
-#else
-  __asm__ __volatile__("" : : : "memory");
-#endif
 #elif defined(__GNUC__)
   /*
-   * __clear_cache() does two things:
+   * For ARM __clear_cache() does two things:
    *
    *  1) It flushes the write buffer for the address range.
    *     We need to do this for writable_addr.

src/trusted/debug_stub/nacl_debug.cc

@@ -28,10 +28,6 @@
 #include "native_client/src/trusted/service_runtime/sel_ldr.h"
 #include "native_client/src/trusted/service_runtime/thread_suspension.h"
 
-#if NACL_OSX
-# include <sys/sysctl.h>
-#endif
-
 using port::IPlatform;
 using port::Thread;
 using port::ITransport;
@@ -120,28 +116,13 @@ static const struct NaClDebugCallbacks debug_callbacks = {
   ProcessExitHook,
 };
 
-#if NACL_OSX
-// From https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment
-static int processIsTranslated() {
-  int ret = 0;
-  size_t size = sizeof(ret);
-  if (sysctlbyname("sysctl.proc_translated", &ret, &size, NULL, 0) == -1)
-  {
-    if (errno == ENOENT)
-      return 0;
-    return -1;
-  }
-  return ret;
-}
-#endif
-
 /*
  * This function is implemented for the service runtime.  The service runtime
  * declares the function so it does not need to be declared in our header.
  */
 int NaClDebugInit(struct NaClApp *nap) {
 #if NACL_OSX
-  if (processIsTranslated() != 0) {
+  if (nap->in_emulator) {
     // Thread suspension facilities don't work
     NaClLog(LOG_ERROR, "NaCl debugging not available under Rosetta translation\n");
     return 0;

src/trusted/service_runtime/nacl_text.c

@@ -31,6 +31,7 @@
 #include "native_client/src/trusted/service_runtime/thread_suspension.h"
 
 #if NACL_OSX
+#include <errno.h>
 #include "native_client/src/trusted/desc/osx/nacl_desc_imc_shm_mach.h"
 #endif
 
@@ -96,6 +97,65 @@ static struct NaClDesc *MakeImcShmDesc(uintptr_t size) {
   return &shm->base;
 }
 
+/*
+ * Toggling PROT_EXEC seems to be the only way to make Rosetta re-translate
+ * pages that have been executed previously. Officially recommended methods
+ * like sys_icache_invalidate don't help. Better hope no untrusted code
+ * is executing there...
+ * The memory range's protection is assumed to start as exec+read.
+ * nap->dynamic_load_mutex should be held.
+ */
+static void RosettaFlushInstructionCache(struct NaClApp *nap,
+                                         uintptr_t executable_addr,
+                                         size_t size) {
+#if NACL_OSX
+  char *start;
+  char *end;
+
+  if (!nap->in_emulator) {
+    return;
+  }
+
+  start = (char *) (executable_addr & ~(nap->page_size - 1));
+  end = (char *) NaClRoundPage(executable_addr + size, nap->page_size);
+
+  if (0 != mprotect(start, end - start, PROT_READ) ||
+      0 != mprotect(start, end - start, PROT_READ | PROT_EXEC)) {
+    NaClLog(LOG_FATAL, "Failed to toggle PROT_EXEC: errno %d\n", errno);
+  }
+#else
+  NACL_UNUSED_PARAMETER(nap);
+  NACL_UNUSED_PARAMETER(executable_addr);
+  NACL_UNUSED_PARAMETER(size);
+#endif
+}
+
+static int NaClCopyCode(struct NaClApp *nap, uintptr_t guest_addr,
+                        uint8_t *exec_addr,
+                        uint8_t *write_addr, uint8_t *replacement_addr,
+                        size_t size) {
+  int status;
+  status = NaClValidateStatus(nap->validator->CopyCode(
+                              guest_addr, write_addr, replacement_addr, size,
+                              nap->cpu_features,
+                              NaClCopyInstruction));
+  /*
+   * Flush the processor's instruction cache.  This is not necessary
+   * for security, because any old cached instructions will just be
+   * safe halt instructions.  It is only necessary to ensure that
+   * untrusted code runs correctly when it tries to execute the
+   * dynamically-loaded code.
+   *
+   * For Rosetta there's no thread syncing in this one so other threads
+   * executing code in the same page could crash upon toggling PROT_EXEC.
+   */
+  NaClFlushCacheForDoublyMappedCode(write_addr,
+                                    exec_addr,
+                                    size);
+  RosettaFlushInstructionCache(nap, (uintptr_t) exec_addr, size);
+  return status;
+}
+
 NaClErrorCode NaClMakeDynamicTextShared(struct NaClApp *nap) {
   uintptr_t                   dynamic_text_size;
   uintptr_t                   shm_vaddr_base;
@@ -754,6 +814,7 @@ int32_t NaClTextDyncodeCreate(struct NaClApp *nap,
    * dynamically-loaded code.
    */
   NaClFlushCacheForDoublyMappedCode(mapped_addr, (uint8_t *) dest_addr, size);
+  RosettaFlushInstructionCache(nap, dest_addr, size);
 
   retval = 0;
 
@@ -921,7 +982,8 @@ int32_t NaClSysDyncodeModify(struct NaClAppThread *natp,
     goto cleanup_unlock;
   }
 
-  if (LOAD_OK != NaClCopyCode(nap, dest, mapped_addr, code_copy, size)) {
+  if (LOAD_OK != NaClCopyCode(nap, dest, (uint8_t *) dest_addr,
+                              mapped_addr, code_copy, size)) {
     NaClLog(1, "NaClSysDyncodeModify: Copying of replacement code failed\n");
     retval = -NACL_ABI_EINVAL;
     goto cleanup_unlock;
@@ -1020,6 +1082,7 @@ int32_t NaClSysDyncodeDelete(struct NaClAppThread *natp,
      * icache.
      */
     NaClFlushCacheForDoublyMappedCode(mapped_addr, (uint8_t *) dest_addr, size);
+    RosettaFlushInstructionCache(nap, dest_addr, size);
 
     NaClTextMapClearCacheIfNeeded(nap, dest, size);
 

src/trusted/service_runtime/sel_ldr.c

@@ -15,6 +15,11 @@
 #include <intrin.h>
 #endif
 
+#if NACL_OSX
+#include <errno.h>
+#include <sys/sysctl.h>
+#endif
+
 #include "native_client/src/include/portability.h"
 #include "native_client/src/include/portability_io.h"
 #include "native_client/src/include/portability_string.h"
@@ -81,6 +86,21 @@ static int CheckPageSize(size_t size) {
 #endif
 }
 
+#if NACL_OSX
+// https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment
+static int ProcessIsTranslated(void) {
+  int ret = 0;
+  size_t size = sizeof(ret);
+  if (sysctlbyname("sysctl.proc_translated", &ret, &size, NULL, 0) == -1)
+  {
+    if (errno == ENOENT)
+      return 0;
+    NaClLog(LOG_FATAL, "Failed to retrieve sysctl.proc_translated\n");
+  }
+  return ret;
+}
+#endif
+
 int NaClAppWithEmptySyscallTableCtor(struct NaClApp *nap) {
   struct NaClDescEffectorLdr  *effp;
   int i;
@@ -270,6 +290,9 @@ int NaClAppWithEmptySyscallTableCtor(struct NaClApp *nap) {
   nap->faulted_thread_fd_write = -1;
 #endif
 
+#if NACL_OSX
+  nap->in_emulator = ProcessIsTranslated();
+#endif
 
 #if NACL_LINUX || NACL_OSX
   /*

src/trusted/service_runtime/sel_ldr.h

@@ -380,6 +380,10 @@ struct NaClApp {
    */
   int sc_nprocessors_onln;
 
+#if NACL_OSX
+  int in_emulator;
+#endif
+
   size_t page_size;
 
   const struct NaClValidatorInterface *validator;
@@ -457,6 +461,8 @@ NaClErrorCode NaClAppLoadFileDynamically(
     struct NaClDesc *ndp,
     struct NaClValidationMetadata *metadata) NACL_WUR;
 
+int NaClValidateStatus(NaClValidationStatus status);
+
 int NaClValidateCode(struct NaClApp *nap,
                      uintptr_t      guest_addr,
                      uint8_t        *data,
@@ -473,13 +479,6 @@ int NaClValidateCodeReplacement(struct    NaClApp *nap,
                                 uint8_t   *data_new,
                                 size_t    size);
 
-/*
- * Copies code from data_new to data_old in a thread-safe way.
- */
-int NaClCopyCode(struct NaClApp *nap, uintptr_t guest_addr,
-                 uint8_t *data_old, uint8_t *data_new,
-                 size_t size);
-
 /*
  * Copies an instruction in a thread-safe way. Used by validators.
  */

src/trusted/service_runtime/sel_validate_image.c

@@ -13,7 +13,7 @@
 const size_t kMinimumCachedCodeSize = 40000;
 
 /* Translate validation status to values wanted by sel_ldr. */
-static int NaClValidateStatus(NaClValidationStatus status) {
+int NaClValidateStatus(NaClValidationStatus status) {
   switch (status) {
     case NaClValidationSucceeded:
       return LOAD_OK;
@@ -98,27 +98,6 @@ int NaClValidateCodeReplacement(struct NaClApp *nap, uintptr_t guest_addr,
       guest_addr, data_old, data_new, size, nap->cpu_features));
 }
 
-int NaClCopyCode(struct NaClApp *nap, uintptr_t guest_addr,
-                 uint8_t *data_old, uint8_t *data_new,
-                 size_t size) {
-  int status;
-  status = NaClValidateStatus(nap->validator->CopyCode(
-                              guest_addr, data_old, data_new, size,
-                              nap->cpu_features,
-                              NaClCopyInstruction));
-  /*
-   * Flush the processor's instruction cache.  This is not necessary
-   * for security, because any old cached instructions will just be
-   * safe halt instructions.  It is only necessary to ensure that
-   * untrusted code runs correctly when it tries to execute the
-   * dynamically-loaded code.
-   */
-  NaClFlushCacheForDoublyMappedCode(data_old,
-                                    (uint8_t *) guest_addr,
-                                    size);
-  return status;
-}
-
 NaClErrorCode NaClValidateImage(struct NaClApp  *nap) {
   uintptr_t               memp;
   uintptr_t               endp;

tests/dynamic_code_loading/dynamic_load_test.c

@@ -21,7 +21,7 @@
 
 #if defined(__x86_64__)
 /* On x86-64, template functions do not fit in 32-byte buffers */
-#define BUF_SIZE 128
+#define BUF_SIZE 64
 #elif defined(__i386__) || defined(__arm__)
 #define BUF_SIZE 32
 #else

tests/dynamic_code_loading/nacl.scons

@@ -30,11 +30,6 @@ if env.Bit('build_mips32'):
 # See http://code.google.com/p/nativeclient/issues/detail?id=1112
 is_broken = not env.Bit('nacl_static_link')
 
-# there is fair amount of assembly code in these tests
-asm_env = env.Clone()
-if env.Bit('bitcode'):
-  asm_env.PNaClForceNative()
-  asm_env.AddBiasForPNaCl()
 if env.Bit('bitcode'):
   # NOTE: we cannot use PNaClForceNative here as we want the
   #       the .c files to actually go to bc files - but this is not
@@ -58,7 +53,25 @@ def GetTemplate(env):
   else:
     return 'templates_x86.S'
 
-template_obj = asm_env.ComponentObject(GetTemplate(env))
+# These are built with the default toolchain using Chromium tools. The Saigo
+# assembler heavily modifies the code making it not work with dynamic_modify_test.
+def GetPrebuiltTemplate(env):
+  if env.Bit('build_arm'):
+    return 'templates_arm.o'
+  if env.Bit('build_x86_32'):
+    return 'templates_x86-32.o'
+  if env.Bit('build_x86_64'):
+    return 'templates_x86-64.o'
+
+if env.Bit('saigo'):
+  template_obj = File(GetPrebuiltTemplate(env))
+else:
+  # there is fair amount of assembly code in these tests
+  asm_env = env.Clone()
+  if env.Bit('bitcode'):
+    asm_env.PNaClForceNative()
+    asm_env.AddBiasForPNaCl()
+  template_obj = asm_env.ComponentObject(GetTemplate(env))
 
 dynamic_load_test_nexe = env.ComponentProgram(
     env.ProgramNameForNmf('dynamic_load_test'),
@@ -179,4 +192,4 @@ node = env.CommandSelLdrTestNacl('dynamic_modify_test.out',
 # shared memory segment.  It does not know to flush its code
 # translation cache.
 env.AddNodeToTestSuite(node, test_suites, 'run_dynamic_modify_test',
-                       is_broken=is_broken or env.Bit('saigo') or env.IsRunningUnderValgrind())
+                       is_broken=is_broken or env.IsRunningUnderValgrind())