Rewrite non-temporal instructions

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..51e2a8e1a98c7ff7cdff8780ae7cd708c689c2b0 100644
--- a/src/trusted/validator_ragel/dfa_validate_common.c
+++ b/src/trusted/validator_ragel/dfa_validate_common.c
@@ -11,7 +11,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 +33,218 @@ Bool NaClDfaProcessValidationError(const uint8_t *begin, const uint8_t *end,
   return FALSE;
 }
 
-Bool NaClDfaStubOutUnsupportedInstruction(const uint8_t *begin,
+Bool NaClDfaProcessPostRewriteValidationError(const uint8_t *begin,
+                                              const uint8_t *end,
+                                              uint32_t info,
+                                              void *callback_data) {
+  UNREFERENCED_PARAMETER(begin);
+  UNREFERENCED_PARAMETER(end);
+  UNREFERENCED_PARAMETER(callback_data);
+  /*
+   * Similar to NaClDfaRewriteUnsupportedInstruction(), we don't consider
+   * DIRECT_JUMP_OUT_OF_RANGE as an error. But if we still get
+   * CPUID_UNSUPPORTED_INSTRUCTION or UNSUPPORTED_INSTRUCTION, the validation
+   * should fail, because these errors should have already been fixed
+   * by NaClDfaRewriteUnsupportedInstruction().
+   */
+  if ((info & VALIDATION_ERRORS_MASK) == DIRECT_JUMP_OUT_OF_RANGE)

[Mark Seaborn, 2015/08/14 21:12:59]

if (X)
  return TRUE;
else
  return FALSE:

can be written more concisely as "return X".

[ruiq, 2015/08/15 04:46:41]

Done.

+    return TRUE;
+  else
+    return FALSE;
+}
+
+#if NACL_BUILD_SUBARCH == 64
+static Bool IsREX(uint8_t byte) {
+  return byte >= 0x40 && byte <= 0x4f;
+}
+#endif
+
+static Bool NaClDfaRewriteUnsupportedInstruction(const uint8_t *begin,

[Mark Seaborn, 2015/08/14 21:12:59]

Could this ever be called with begin == end?  Should we have a CHECK to prevent
that?

                                           const uint8_t *end,

[Mark Seaborn, 2015/08/14 21:12:59]

Fix indentation to line up

[ruiq, 2015/08/15 04:46:41]

Done.

                                           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) {
+  /*
+   * Clear DIRECT_JUMP_OUT_OF_RANGE error, because it may be introduced by
+   * validating a bundle which is smaller than the original chunk size. Even if
+   * the orignal chunk has this error, it can be detected when validating the
+   * whole chunk.
+   */
+  info &= ~DIRECT_JUMP_OUT_OF_RANGE;
+  if ((info & VALIDATION_ERRORS_MASK) == 0) {
+    return TRUE;
+  } else if ((info & VALIDATION_ERRORS_MASK) == CPUID_UNSUPPORTED_INSTRUCTION) {
+    /* 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;
   }
+  /*
+   * Instruction rewriting. Note that we only rewrite non-temporal instructions
+   * found in current webstore nexes so that validation succeeds and we don't
+   * break them. If future nexes use other non-temporal instructions, they will
+   * fail validation.
+   *
+   * 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

[Mark Seaborn, 2015/08/14 21:12:59]

I believe you can use if() rather than #if here.

if() is preferred when possible.  See:
http://lackingrhoticity.blogspot.com/2015/01/conditionalising-c-ifdef-vs-if.html

+  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)) {

[Mark Seaborn, 2015/08/14 21:12:59]

Nit: don't need ()s around "begin[1] == 0x0f"

[ruiq, 2015/08/15 04:46:41]

Done.

+    uint8_t opcode_byte2 = begin[2];
+    switch (opcode_byte2) {
+      case 0x2b:
+        /* movntps => movaps */
+        ptr[2] = 0x29;

[Mark Seaborn, 2015/08/14 21:12:59]

It's surprising that you're using "begin" to read, but "ptr" to write.  Can you
make it consistent?

[ruiq, 2015/08/15 04:46:41]

Done.

+        data->did_rewrite = 1;
+        return TRUE;
+      case 0xc3:
+        /* movnti => mov, nop */
+        if (info & RESTRICTED_REGISTER_USED) {
+          /*
+           * The rewriting for movnti is special because it changes instruction
+           * boundary: movnti is replaced by a mov and a nop so that the total
+           * size does not change. Therefore, special care needs to be taken:
+           * if restricted register is used in this instruction, we have to put
+           * nop at the end so that the rewritten restricted register consuming
+           * instruction follows closely with the restricted register producing
+           * instruction (if there is one).
+           */
+          ptr[1] = 0x89;
+          memmove(ptr + 2, ptr + 3, end - begin - 3);
+          ptr[end - begin - 1] = 0x90;

[Mark Seaborn, 2015/08/14 21:12:59]

How about commenting this as: 0x90; /* NOP */

Same below.

[ruiq, 2015/08/15 04:46:41]

Done.

+        } else {
+          /*
+           * There are cases where we need to preserve instruction end position,
+           * for example, when RIP-relative address is used. Fortunately, RIP-
+           * relative addressing cannot use an index register, and therefore
+           * RESTRICTED_REGISTER_USED cannot be set. Therefore, no matter
+           * whether RIP-relative addressing is used, as long as restricted
+           * register is not used, we are safe to put nop in the beginning and
+           * preserve instruction end position.
+           */
+          ptr[2] = 0x89;
+          ptr[1] = ptr[0];
+          ptr[0] = 0x90;
+        }
+        data->did_rewrite = 1;
+        return TRUE;
+      case 0x18:
+        /* prefetchnta => nop...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) {

[Mark Seaborn, 2015/08/14 21:12:59]

Fix indentation alignment

[ruiq, 2015/08/15 04:46:41]

Done.

+    /* movntdq => movdqa */
+    ptr[3] = 0x7f;
+    data->did_rewrite = 1;
+    return TRUE;
+  }
+#endif

[Mark Seaborn, 2015/08/14 21:12:59]

For safety, add:
#else
# error Unknown architecture

[ruiq, 2015/08/15 04:46:41]

Done.

+  return FALSE;
+}
+
+Bool NaClDfaStubOutUnsupportedInstruction(const uint8_t *begin,
+                                          const uint8_t *end,
+                                          uint32_t info,
+                                          void *callback_data) {
+  struct StubOutCallbackData *data = callback_data;
+  uintptr_t temp_addr;
+  uint8_t *bundle_begin;
+  Bool rc;
+  UNREFERENCED_PARAMETER(end);
+
+  /*
+   * We can only handle two types of errors by rewriting:
+   * CPUID_UNSUPPORTED_INSTRUCTION and UNSUPPORTED_INSTRUCTION.
+   */
+  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);

[Mark Seaborn, 2015/08/14 21:12:59]

I'm not sure I understand the purpose of this parameter if it's always supposed
to be false...

+  /*
+   * Compute bundle begin. Note that the validator does not enforce the
+   * validated chunk to start at a 32-byte aligned address, although it checks
+   * the chunk size to be a multiple of 32 (kBundleSize). Therefore, we cannot
+   * simply compute bundle_begin from "begin & ~kBundleMask", but have to
+   * consider to bundle_begin_offset. Alternatively, bundle begin can be passed
+   * to the user callback by the validator. However, the callback interface
+   * needs to be changed for this additional parameter, because "callback_data"
+   * is opaque to the validator and should not be used for this purpose.
+   */
+  temp_addr = ((uintptr_t) begin & ~kBundleMask) + data->bundle_begin_offset;
+  if (temp_addr <= (uintptr_t) begin)
+    bundle_begin = (uint8_t *) temp_addr;
+  else
+    bundle_begin = (uint8_t *) (temp_addr - kBundleSize);
+
+  /*
+   * Rewrite the bundle containing current instruction. We want to rewrite the
+   * whole bundle primarily because this eases revalidation. We should not
+   * revalidate a single instruction because it may use a restricted register,
+   * and is prefixed by a restricted register producing instruction in the
+   * bundle. If we just revalidate this restricted register consuming
+   * instruction, we will erroneously get an UNRESTRICTED_INDEX_REGISTER error.
+   * On the other hand, we don't want to revalidate the whole chunk because it
+   * can be expensive. Therefore, the only choice left is to revalidate the
+   * current bundle. This requires us to rewrite the whole bundle first, because
+   * there could be to-be-rewritten instructions after current instruction, and
+   * the revalidation of current bundle would fail if we just rewrite current
+   * instruction without rewriting those after.
+   */
+#if NACL_BUILD_SUBARCH == 32
+  rc = ValidateChunkIA32(bundle_begin, kBundleSize, 0 /*options*/,
+                         data->cpu_features,
+                         NaClDfaRewriteUnsupportedInstruction,
+                         callback_data);
+#elif NACL_BUILD_SUBARCH == 64
+  rc = ValidateChunkAMD64(bundle_begin, kBundleSize, 0 /*options*/,
+                          data->cpu_features,
+                          NaClDfaRewriteUnsupportedInstruction,
+                          callback_data);
+#endif
+
+  if (!rc)
+    return FALSE;
+
+  /* Revalidate the bundle after rewriting. */
+#if NACL_BUILD_SUBARCH == 32
+  rc = ValidateChunkIA32(bundle_begin, kBundleSize, 0 /*options*/,
+                          data->cpu_features,
+                          NaClDfaProcessPostRewriteValidationError,
+                          NULL);
+#elif NACL_BUILD_SUBARCH == 64
+  rc = ValidateChunkAMD64(bundle_begin, kBundleSize, 0 /*options*/,
+                           data->cpu_features,
+                           NaClDfaProcessPostRewriteValidationError,
+                           NULL);
+#endif
+  return rc;
 }
 
 Bool NaClDfaProcessCodeCopyInstruction(const uint8_t *begin_new,