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..7fc891f650dc7086c78a1f52c7718a069ffc4229 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,212 @@ 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

[Petr Hosek, 2015/08/11 18:22:35]

Nit: the comment body should only start on the second line:

/*
 * ...
 */

[ruiq, 2015/08/11 21:08:34]

Done.

+   * 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)
+    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,
                                           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) {
+  /* 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
+  UNREFERENCED_PARAMETER(end);
+  if (memcmp(begin, "\x0f\xe7", 2) == 0) {

[Petr Hosek, 2015/08/11 18:22:36]

Since you already have a `ptr`, you could first check the prefix, i.e.:

if (*ptr == 0x66)
  ptr++;

Then you can combine both these cases into one, i.e.:

if (memcmp(ptr, "\x0f\xe7", 2) == 0) {
  ...
}

[ruiq, 2015/08/11 21:08:34]

Separating these two cases were done on purpose. The idea was to make it more
explicit what instruction is rewritten to what (see the comments above each
case).

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

[Petr Hosek, 2015/08/11 18:22:36]

The same here, you can use `ptr` to simplify this code (you'll need to use the
`begin` for the actual rewriting or introduce another helper variable).

[ruiq, 2015/08/11 21:08:34]

Same here.

+    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, 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;
+        } 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) {

[Petr Hosek, 2015/08/11 18:22:35]

This could be merged into the `if` case.

[ruiq, 2015/08/11 21:08:34]

Same here.

[ruiq, 2015/08/12 05:37:50]

The other point is that we don't want to overly generalize the rewriting. For
example, we want to rewrite "66 REX 0f e7", but we don't want to rewrite e.g.,
"66 REX 0f 2b".

+    /* 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;
+  uintptr_t temp_addr;
+  uint8_t *bundle_begin;
+  Bool rc, rc2;

[Petr Hosek, 2015/08/11 18:22:36]

You don't need two different variable for return value.

[ruiq, 2015/08/11 21:08:33]

Done.

+  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);
+  /* 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
+  rc2 = ValidateChunkIA32(bundle_begin, kBundleSize, 0 /*options*/,
+                          data->cpu_features,
+                          NaClDfaProcessPostRewriteValidationError,
+                          NULL);
+#elif NACL_BUILD_SUBARCH == 64
+  rc2 = ValidateChunkAMD64(bundle_begin, kBundleSize, 0 /*options*/,
+                           data->cpu_features,
+                           NaClDfaProcessPostRewriteValidationError,
+                           NULL);
+#endif
+  if (!rc2)
+    return FALSE;
+  return TRUE;

[Petr Hosek, 2015/08/11 18:22:36]

Nit: you can just return `rc` here.

[ruiq, 2015/08/11 21:08:34]

Done.

 }
 
 Bool NaClDfaProcessCodeCopyInstruction(const uint8_t *begin_new,