Experimental: Bundle revalidation inside user callback

src/trusted/validator_ragel/dfa_validate_common.c

Index: src/trusted/validator_ragel/dfa_validate_common.c
diff --git a/src/trusted/validator_ragel/dfa_validate_common.c b/src/trusted/validator_ragel/dfa_validate_common.c
index 4972cbd654b3fcd0d884dec0099db63750991802..477cf6fa8add1b8a8beb17ba93e16f126a7da729 100644
--- a/src/trusted/validator_ragel/dfa_validate_common.c
+++ b/src/trusted/validator_ragel/dfa_validate_common.c
@@ -12,6 +12,7 @@
 #include "native_client/src/shared/platform/nacl_check.h"
 #include "native_client/src/trusted/service_runtime/nacl_config.h"
 #include "native_client/src/trusted/validator_ragel/validator.h"
+#include "native_client/src/include/build_config.h"
 
 /* Used as an argument to copy_func when unsupported instruction must be
    replaced with HLTs.  */
@@ -33,29 +34,134 @@ Bool NaClDfaProcessValidationError(const uint8_t *begin, const uint8_t *end,
   return FALSE;
 }
 
-Bool NaClDfaStubOutUnsupportedInstruction(const uint8_t *begin,
+#if NACL_BUILD_SUBARCH == 64
+static Bool IsREX(uint8_t byte) {
+  return byte >= 0x40 && byte <= 0x4f;
+}
+#endif
+
+static Bool NaClRewriteUnsupportedInstruction(const uint8_t *begin,
                                           const uint8_t *end,
                                           uint32_t info,
                                           void *callback_data) {
+  uint8_t *ptr = (uint8_t *) begin;
   struct StubOutCallbackData *data = callback_data;
-  /* Stub-out instructions unsupported on this CPU, but valid on other CPUs.  */
   if ((info & VALIDATION_ERRORS_MASK) == CPUID_UNSUPPORTED_INSTRUCTION) {

[khimg, 2015/08/10 00:51:43]

Instead of VALIDATION_ERRORS_MASK we must use (VALIDATION_ERRORS_MASK &
~DIRECT_JUMP_OUT_OF_RANGE) - although some symbolic name for it will be good.

Otherwise jumps to unaligned addresses will erroneously trigger error here.
High-level validator will check these addresses for us.

+    /* Stub-out instructions unsupported on this CPU, but valid on other CPUs.*/
     data->did_rewrite = 1;
     memset((uint8_t *)begin, NACL_HALT_OPCODE, end - begin);
     return TRUE;
-  } else if ((info & VALIDATION_ERRORS_MASK) == UNSUPPORTED_INSTRUCTION) {
-    if (data->flags & NACL_DISABLE_NONTEMPORALS_X86) {
-      return FALSE;
-    } else {
-      /* TODO(ruiq): rewrite instruction. For now, we keep the original
-       * instruction and indicate validation success, which is consistent
-       * with current validation results. */
-      data->did_rewrite = 0;
-      return TRUE;
-    }
-  } else {
+  } else if ((info & VALIDATION_ERRORS_MASK) != UNSUPPORTED_INSTRUCTION) {
     return FALSE;
   }
+  /* We usually only check and rewrite the first few bytes without examining
+   * further because this function is only called when the validator tells us
+   * that it is an 'unsupported instruction' and there are no other validation
+   * failures.
+   */
+#if NACL_BUILD_SUBARCH == 32
+  UNREFERENCED_PARAMETER(end);
+  if (memcmp(begin, "\x0f\xe7", 2) == 0) {
+    /* movntq => movq */
+    ptr[1] = 0x7f;
+    data->did_rewrite = 1;
+    return TRUE;
+  } else if (memcmp(begin, "\x66\x0f\xe7", 3) == 0) {
+    /* movntdq => movdqa */
+    ptr[2] = 0x7f;
+    data->did_rewrite = 1;
+    return TRUE;
+  }
+#elif NACL_BUILD_SUBARCH == 64
+  if (IsREX(begin[0]) && (begin[1] == 0x0f)) {
+    uint8_t opcode_byte2 = begin[2];
+    switch (opcode_byte2) {
+      case 0x2b:
+        /* movntps => movaps */
+        ptr[2] = 0x29;
+        data->did_rewrite = 1;
+        return TRUE;
+      case 0xc3:
+        /* movnti => mov */
+        ptr[1] = 0x89;
+        memmove(ptr + 2, ptr + 3, end - begin - 3);
+        ptr[end - begin - 1] = 0x90;

[khimg, 2015/08/10 00:51:43]

Please insert NOP before mov, not after! Think about %rip!

+        data->did_rewrite = 1;
+        return TRUE;
+      case 0x18:
+        /* prefetchnta => nop */
+        memset(ptr, 0x90, end - begin);
+        data->did_rewrite = 1;
+        return TRUE;
+      default:
+        return FALSE;
+    }
+  } else if (begin[0] == 0x66 && IsREX(begin[1]) &&
+      memcmp(begin + 2, "\x0f\xe7", 2) == 0) {
+    /* movntdq => movdqa */
+    ptr[3] = 0x7f;
+    data->did_rewrite = 1;
+    return TRUE;
+  }
+#endif
+  return FALSE;
+}
+
+Bool NaClDfaStubOutUnsupportedInstruction(const uint8_t *begin,
+                                          const uint8_t *end,
+                                          uint32_t info,
+                                          void *callback_data) {
+  struct StubOutCallbackData *data = callback_data;
+  struct StubOutCallbackData reval_data = *data;
+  intptr_t addr;
+  uint8_t *bundle_begin;
+  Bool rc, rc2;
+  UNREFERENCED_PARAMETER(end);
+
+  if ((info & VALIDATION_ERRORS_MASK) != CPUID_UNSUPPORTED_INSTRUCTION &&
+      (info & VALIDATION_ERRORS_MASK) != UNSUPPORTED_INSTRUCTION)
+    return FALSE;
+  if ((info & VALIDATION_ERRORS_MASK) == UNSUPPORTED_INSTRUCTION &&
+      (data->flags & NACL_DISABLE_NONTEMPORALS_X86))
+    return FALSE;
+
+  CHECK(!data->chunk_processed_as_a_contiguous_stream);
+  addr = ((intptr_t) begin & ~(kBundleMask)) + data->bundle_begin_offset;
+  if (addr > (intptr_t) begin)
+    bundle_begin  = (uint8_t *) (addr - kBundleSize);
+  else
+    bundle_begin = (uint8_t *) addr;

[ruiq, 2015/08/10 00:26:08]

We need to compute bundle_begin this way because the validator does not enforce
the start of the chunk to be 32-byte aligned (although it validates bundle by
bundle and enforces that the size is a multiple of kBundleSize). Moreover, some
of the tests (e.g., src/trusted/validator/validation_cache_test.cc) pass in a
code chunk address which is not necessarily 32-byte aligned. Therefore, what we
get
from "begin & ~(kBundleMask)" is not necessarily the start of the bundle, and we
have to do the above calculation. Alternatively, the bundle start can be passed
to the user callback by the validator. However, in that case the user callback
interface needs to be changed to add an additional parameter, because
"callback_data" is opaque to validator and cannot be used for this purpose. 

+  /* Rewrite a bundle at a time. */
+#if NACL_BUILD_SUBARCH == 32
+  rc = ValidateChunkIA32(bundle_begin, kBundleSize, 0 /*options*/,
+      (NaClCPUFeaturesX86 *) data->cpu_features,
+      NaClRewriteUnsupportedInstruction,
+      callback_data);
+#elif NACL_BUILD_SUBARCH == 64
+  rc = ValidateChunkAMD64(bundle_begin, kBundleSize, 0 /*options*/,
+      (NaClCPUFeaturesX86 *) data->cpu_features,
+      NaClRewriteUnsupportedInstruction,
+      callback_data);
+#endif
+
+  if (!rc)
+    return FALSE;
+
+  /* Revalidate the bundle after rewriting. */
+#if NACL_BUILD_SUBARCH == 32
+  rc2 = ValidateChunkIA32(bundle_begin, kBundleSize, 0 /*options*/,
+      (NaClCPUFeaturesX86 *) data->cpu_features,
+      NaClRewriteUnsupportedInstruction,
+      &reval_data);
+#elif NACL_BUILD_SUBARCH == 64
+  rc2 = ValidateChunkAMD64(bundle_begin, kBundleSize, 0 /*options*/,
+      (NaClCPUFeaturesX86 *) data->cpu_features,
+      NaClRewriteUnsupportedInstruction,
+      &reval_data);
+#endif
+  if (!rc2 || reval_data.did_rewrite)
+    return FALSE;
+  return TRUE;
 }
 
 Bool NaClDfaProcessCodeCopyInstruction(const uint8_t *begin_new,