Resurrect Petr's 64-bit dynamic code modification CL:...

src/trusted/validator_x86/ncvalidate_iter.c

 /*
  * Copyright 2009 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can
  * be found in the LICENSE file.
  */
 
 /*
  * ncvalidate_iter.c
  * Validate x86 instructions for Native Client
  *
  */
 
 #include <assert.h>
 #include <string.h>
 
 #include "native_client/src/trusted/validator_x86/ncvalidate_iter.h"
-
+#include "native_client/src/shared/platform/nacl_check.h"
 #include "native_client/src/shared/platform/nacl_log.h"
 #include "native_client/src/trusted/validator_x86/nc_inst_iter.h"
 #include "native_client/src/trusted/validator_x86/nc_inst_state_internal.h"
 #include "native_client/src/trusted/validator_x86/nc_segment.h"
 #include "native_client/src/trusted/validator_x86/ncop_exps.h"
 #include "native_client/src/trusted/validator_x86/ncvalidate_iter_internal.h"
 #include "native_client/src/trusted/validator_x86/ncvalidator_registry.h"
 
 /* To turn on debugging of instruction decoding, change value of
  * DEBUGGING to 1.
@@ skipping 224 matching lines @@
     NaClLogV_mu(level, format, ap);
     va_end(ap);
     NaClLogUnlock();
     NaClRecordErrorReported(state, level);
   }
   if (state->do_stub_out && (level <= LOG_ERROR)) {
     memset(inst->mpc, kNaClFullStop, inst->length);
   }
 }
 
+void NaClValidatorTwoInstMessage(int level,
+                                 NaClValidatorState* state,
+                                 NaClInstState* inst1,
+                                 NaClInstState* inst2,
+                                 const char* format,
+                                 ...) {
+  level = NaClRecordIfValidatorError(state, level);
+  if (NaClPrintValidatorMessages(state, level)) {
+    va_list ap;
+    struct Gio* g = NaClLogGetGio();
+
+    NaClLogLock();
+    /* TODO(karl) - Make printing of instruction state possible via format. */
+    va_start(ap, format);
+    NaClLog_mu(level, "VALIDATOR: %s", NaClLogLevelLabel(level));
+    NaClLogV_mu(level, format, ap);
+    va_end(ap);
+
+    /* TODO(karl): empty fmt strings not allowed */
+    NaClLog_mu(level, "\n%45s ", "VALIDATOR:");
+    NaClInstStateInstPrint(g, inst1);
+    /* TODO(karl): empty fmt strings not allowed */
+    NaClLog_mu(level, "%45s ", "VALIDATOR:");
+    NaClInstStateInstPrint(g, inst2);
+
+    NaClLogUnlock();
+    NaClRecordErrorReported(state, level);
+  }
+  if (state->do_stub_out && (level <= LOG_ERROR)) {

[bsy, 2010/12/15 04:06:50]

this appears to be dead code, since do_stub_out must be false in order to not
early exit in NaClValidateCodeReplacement, which so far is the only way to get
to NaClValidatorTwoInstMessage.  We should probably replace this with a comment
that we don't stub out when it's a two inst streams.

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

I'm very unfamiliar with this, so I haven't done this yet... what is do_stub_out
anyway?

On 2010/12/15 04:06:50, bsy wrote:

+    memset(inst1->mpc, kNaClFullStop, inst1->length);
+    memset(inst2->mpc, kNaClFullStop, inst2->length);
+  }
+}
+
 Bool NaClValidatorQuit(NaClValidatorState* state) {
   return !state->validates_ok && (state->quit_after_error_count == 0);
 }
 
 void NaClRegisterValidator(
     NaClValidatorState* state,
     NaClValidator validator,
     NaClValidatorPostValidate post_validate,
     NaClValidatorPrintStats print_stats,
     NaClValidatorMemoryCreate create_memory,
@@ skipping 171 matching lines @@
 void* NaClGetValidatorLocalMemory(NaClValidator validator,
                                   const NaClValidatorState* state) {
   int i;
   for (i = 0; i < state->number_validators; ++i) {
     if (state->validators[i].validator == validator) {
       return state->local_memory[i];
     }
   }
   return NULL;
 }
+
+/*
+ * Check that iter_new is a valid replacement for iter_old.
+ * If a validation error occurs, state->validates_ok will be set to false by
+ * NaClValidatorInstMessage when it is given LOG_ERROR, see the end of this
+ * function.
+ */
+static void NaClValidateInstReplacement(NaClInstIter* iter_old,
+                                        NaClInstIter* iter_new,
+                                        struct NaClValidatorState* state) {
+  NaClInstState *istate_old, *istate_new;
+  NaClExpVector *exp_old, *exp_new;
+  uint32_t i;
+  Bool error_in_nodes;
+
+  istate_old = NaClInstIterGetState(iter_old);
+  istate_new = NaClInstIterGetState(iter_new);
+
+  /* Location/length must match */
+  if (istate_new->vpc != istate_old->vpc ||
+      istate_new->length != istate_old->length) {
+    NaClValidatorTwoInstMessage(LOG_ERROR, state, istate_old, istate_new,
+          "Code modification: instructions length/addresses do not match");
+    return;
+  }
+
+
+  do {
+    /* fast check if the replacement is identical */
+    if (!memcmp(istate_old->mpc, istate_new->mpc, istate_old->length))
+      return;
+
+    if (istate_old->num_prefix_bytes != istate_new->num_prefix_bytes)
+      break;
+    if (istate_old->num_rex_prefixes != istate_new->num_rex_prefixes)
+      break;
+    if (istate_old->rexprefix != istate_new->rexprefix)
+      break;
+    if (istate_old->modrm != istate_new->modrm)
+      break;
+    if (istate_old->has_sib != istate_new->has_sib)
+      break;
+    if (istate_old->has_sib && istate_old->sib != istate_new->sib)
+      break;
+    if (istate_old->operand_size != istate_new->operand_size)
+      break;
+    if (istate_old->address_size != istate_new->address_size)
+      break;
+    if (istate_old->prefix_mask != istate_new->prefix_mask)
+      break;
+
+    /*
+     * these are pointers, but they reference entries in a static table,
+     * so if the two instructions are the same, then these pointers must
+     * reference the same entry
+     */
+    if (istate_old->inst != istate_new->inst)
+      break;
+
+    exp_old = NaClInstStateExpVector(istate_old);
+    exp_new = NaClInstStateExpVector(istate_new);
+
+    /* check if the instruction operands are identical */
+    if (exp_old->number_expr_nodes != exp_new->number_expr_nodes)
+      break;
+
+    error_in_nodes = FALSE;
+    for (i = 0; i < exp_old->number_expr_nodes; i++) {
+      if (exp_old->node[i].kind  != exp_new->node[i].kind) {
+        error_in_nodes = TRUE;

[bsy, 2010/12/15 04:06:50]

consider using a goto for the double break here and below, since that's clearer
what the intent was.  (it would have avoided the bug that you found!)

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

I am loath to "goto", but I agree that's clearer in this case. Done.

On 2010/12/15 04:06:50, bsy wrote:

+        break;
+      }
+      if (exp_old->node[i].flags != exp_new->node[i].flags) {
+        error_in_nodes = TRUE;
+        break;
+      }
+
+      /*
+       * allow some constants to be different; however it is important not to
+       * allow modification of sandboxing instructions. Note neither of the
+       * instructions allowed for modification is used for sandboxing
+       */
+      if (exp_old->node[i].value != exp_new->node[i].value) {
+        if (exp_old->node[i].kind == ExprConstant) {
+
+          /* allow different constants in direct calls */
+          if (istate_old->inst->name == InstCall)
+            if (exp_old->node[i].flags & NACL_EFLAG(ExprJumpTarget))
+              if (exp_old->node[NaClGetExpParentIndex(exp_old, i)].kind
+                    == OperandReference)
+                continue;
+
+          /*
+           * allow different constants in operand of mov
+           * e.g. mov $rax, 0xdeadbeef
+           */
+          if (istate_old->inst->name == InstMov)
+            if (exp_old->node[i].flags & NACL_EFLAG(ExprUsed))
+              if (exp_old->node[NaClGetExpParentIndex(exp_old, i)].kind
+                    == OperandReference)
+                continue;
+          /*
+           * allow different displacements in memory reference of mov
+           * instructions e.g. mov $rax, [$r15+$rbx*2+0x7fff]
+           */
+          if (istate_old->inst->name == InstMov)
+            if (exp_old->node[NaClGetExpParentIndex(exp_old, i)].kind
+                    == ExprMemOffset)
+              /* displacement is the fourth node after ExprMemOffset node */
+              if (i - NaClGetExpParentIndex(exp_old, i) == 4)

[bsy, 2010/12/15 04:06:50]

magic number -- do we need/want a kDisplacementNode or something?  the e.g.
above might be good enough.

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

Not done.  I consider the comment good enough to explain this, particularly if
we don't have a full set of constants that explain every node index type.

On 2010/12/15 04:06:50, bsy wrote:

+                continue;
+        }
+        error_in_nodes = TRUE;
+        break;
+      }
+    }
+
+    if (error_in_nodes)

[bsy, 2010/12/15 04:06:50]

good catch!  NaClValidatorTwoInstMessage -> NaClRecordIfValidatorError is
required to set valdates_ok to FALSE.  since this style of using a logging
function to record critical info is somewhat non-obvious, a comment here (or
elsewhere in this file) would be good.

for extra extra goodness, ensure we have a test case that this check catches,
but wouldn't otherwise.

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

Commented both locations (valid return and error return)

On 2010/12/15 04:06:50, bsy wrote:

+      break;
+
+    return;
+  } while (0);
+
+  NaClValidatorTwoInstMessage(LOG_ERROR, state, istate_old, istate_new,
+                      "Code modification: failed to modify instruction");
+}
+
+void NaClValidateSegmentPair(uint8_t* mbase_old, uint8_t* mbase_new,

[bsy, 2010/12/15 04:06:50]

a comment here about assumptions -- that vbase/size are 0
mod bundle_size -- would be good.  o/w the buffer might end in the middle of an
atomic code sequence, and the pairwise validation might not catch that the last
instruction is a mask instruction that cannot be changed.

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

Done.

On 2010/12/15 04:06:50, bsy wrote:

+                           NaClPcAddress vbase, size_t size,
+                           struct NaClValidatorState* state) {
+  NaClSegment segment_old, segment_new;
+  NaClInstIter *iter_old, *iter_new;
+
+  NaClValidatorStateInitializeValidators(state);
+  NaClSegmentInitialize(mbase_old, vbase, size, &segment_old);
+  NaClSegmentInitialize(mbase_new, vbase, size, &segment_new);
+  iter_old = NaClInstIterCreateWithLookback(&segment_old, kLookbackSize);
+  iter_new = NaClInstIterCreateWithLookback(&segment_new, kLookbackSize);
+  while (NaClInstIterHasNext(iter_old) &&
+         NaClInstIterHasNext(iter_new)) {
+    state->cur_inst_state = NaClInstIterGetState(iter_new);
+    state->cur_inst = NaClInstStateInst(state->cur_inst_state);
+    state->cur_inst_vector = NaClInstStateExpVector(state->cur_inst_state);
+    NaClApplyValidators(state, iter_new);
+    NaClValidateInstReplacement(iter_old, iter_new, state);
+    if (state->quit) break;
+    NaClInstIterAdvance(iter_old);
+    NaClInstIterAdvance(iter_new);
+  }
+
+  if (NaClInstIterHasNext(iter_old) ||
+      NaClInstIterHasNext(iter_new)) {
+    NaClValidatorMessage(LOG_ERROR, state,
+    "Code modification: code segments have different number of instructions\n");
+  }
+
+  state->cur_inst_state = NULL;
+  state->cur_inst = NULL;
+  state->cur_inst_vector = NULL;
+  NaClApplyPostValidators(state, iter_new);
+  NaClInstIterDestroy(iter_old);
+  NaClInstIterDestroy(iter_new);
+  NaClValidatorStatePrintStats(state);
+}
+