emulate: Abort decoding if opcode is unsupported

When decoding an instruction with an unsupported opcode (indicated
by the INSN_NOTIMPL flag), em_decode_insn() does not fail, which
can lead to a disaster in em_emulate_insn(), e.g. calling an
invalid handler function (soft_handler == NULL) and causing a host
kernel panic (#93).

1. As soon as the opcode is decoded, check if it is unsupported.
   If so, return a fatal error, raise a vCPU panic, and log the
   raw bytes of the instruction.
2. In em_emulate_insn(), make sure soft_handler is valid before
   calling it.
3. Before decoding a new instruction, reset the emulation context,
   so the old context is not accidentally referred to.
4. Add a unit test for the unsupported opcode case. This requires
   refactoring EmulatorTest::run() first.

Signed-off-by: Yu Ning <[email protected]>

core/emulate.c

@@ -1001,6 +1001,10 @@ em_status_t EMCALL em_decode_insn(struct em_context_t *ctxt, const uint8_t *insn
 
     /* Intel SDM Vol. 2A: 2.1.3 ModR/M and SIB Bytes */
     flags = opcode->flags;
+    if (flags & INSN_NOTIMPL) {
+        return EM_ERROR;
+    }
+
     if (flags & INSN_MODRM) {
         ctxt->modrm.value = insn_fetch_u8(ctxt);
     }
@@ -1075,6 +1079,10 @@ em_status_t EMCALL em_emulate_insn(struct em_context_t *ctxt)
     } else {
         void (*soft_handler)(em_context_t*);
         soft_handler = opcode->handler;
+        if (!soft_handler) {
+            rc = EM_ERROR;
+            goto exit;
+        }
         soft_handler(ctxt);
     }
 

core/vcpu.c

@@ -1504,7 +1504,7 @@ int vcpu_execute(struct vcpu_t *vcpu)
     if (!em_ctxt->finished) {
         rc = em_emulate_insn(em_ctxt);
         if (rc < 0) {
-            hax_panic_vcpu(vcpu, "%s: em_emulate_insn() failed: vcpu_id=%u",
+            hax_panic_vcpu(vcpu, "%s: em_emulate_insn() failed: vcpu_id=%u\n",
                            __func__, vcpu->vcpu_id);
             err = HAX_RESUME;
             goto out;
@@ -1956,6 +1956,10 @@ static int vcpu_emulate_insn(struct vcpu_t *vcpu)
     uint64 rip = vcpu->state->_rip;
     uint64 va;
 
+    // Clean up the emulation context of the previous MMIO instruction, so that
+    // even if things go wrong, the behavior will still be predictable.
+    vcpu_init_emulator(vcpu);
+
     // Detect guest mode
     if (!(vcpu->state->_cr0 & CR0_PE))
         mode = EM_MODE_REAL;
@@ -1991,8 +1995,11 @@ static int vcpu_emulate_insn(struct vcpu_t *vcpu)
     em_ctxt->rip = rip;
     rc = em_decode_insn(em_ctxt, instr);
     if (rc < 0) {
-        hax_panic_vcpu(vcpu, "%s: em_decode_insn() failed: vcpu_id=%u",
-                       __func__, vcpu->vcpu_id);
+        hax_panic_vcpu(vcpu, "em_decode_insn() failed: vcpu_id=%u,"
+                       " len=%u, CS:IP=0x%llx:0x%llx, instr[0..5]="
+                       "0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n", vcpu->vcpu_id,
+                       vcpu->vmx.exit_instr_length, cs_base, rip, instr[0],
+                       instr[1], instr[2], instr[3], instr[4], instr[5]);
         return HAX_RESUME;
     }
     if (em_ctxt->len != vcpu->vmx.exit_instr_length) {
@@ -2004,8 +2011,11 @@ static int vcpu_emulate_insn(struct vcpu_t *vcpu)
     }
     rc = em_emulate_insn(em_ctxt);
     if (rc < 0) {
-        hax_panic_vcpu(vcpu, "%s: em_emulate_insn() failed: vcpu_id=%u",
-                       __func__, vcpu->vcpu_id);
+        hax_panic_vcpu(vcpu, "em_emulate_insn() failed: vcpu_id=%u,"
+                       " len=%u, CS:IP=0x%llx:0x%llx, instr[0..5]="
+                       "0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n", vcpu->vcpu_id,
+                       vcpu->vmx.exit_instr_length, cs_base, rip, instr[0],
+                       instr[1], instr[2], instr[3], instr[4], instr[5]);
         return HAX_RESUME;
     }
     return HAX_EXIT;
@@ -2156,6 +2166,8 @@ static const struct em_vcpu_ops_t em_ops = {
 static void vcpu_init_emulator(struct vcpu_t *vcpu)
 {
     struct em_context_t *em_ctxt = &vcpu->emulate_ctxt;
+
+    memset(em_ctxt, 0, sizeof(*em_ctxt));
     em_ctxt->vcpu = vcpu;
     em_ctxt->ops = &em_ops;
     em_ctxt->finished = true;

tests/test_emulator.cpp

@@ -164,28 +164,43 @@ class EmulatorTest : public testing::Test {
         em_ctxt.ops = &em_ops;
         em_ctxt.mode = EM_MODE_PROT64;
         em_ctxt.vcpu = &vcpu;
+        em_ctxt.rip = 0;
+    }
+
+    void assemble_decode(const char* insn,
+                         uint64_t rip,
+                         size_t* size,
+                         size_t* count,
+                         em_status_t* decode_status) {
+        uint8_t* code;
+        int err;
+
+        err = ks_asm(ks, insn, 0, &code, size, count);
+        ASSERT_TRUE(err == 0);
+        EXPECT_TRUE(*size != 0);
+        EXPECT_TRUE(*count != 0);
+
+        em_ctxt.rip = rip;
+        *decode_status = em_decode_insn(&em_ctxt, code);
+        // code == em_ctxt->insn should never be used after em_decode_insn()
+        ks_free(code);
     }
 
     void run(const char* insn,
              const test_cpu_t& vcpu_original,
              const test_cpu_t& vcpu_expected) {
-        uint8_t* code;
         size_t count;
         size_t size;
-        int err;
+        em_status_t ret = EM_ERROR;
 
         vcpu = vcpu_original;
-        err = ks_asm(ks, insn, 0, &code, &size, &count);
-        ASSERT_FALSE(err);
-        em_ctxt.rip = 0;
-        err = em_decode_insn(&em_ctxt, code);
-        ASSERT_TRUE(err != EM_ERROR);
-        err = em_emulate_insn(&em_ctxt);
-        ASSERT_TRUE(err != EM_ERROR);
-        EXPECT_TRUE(vcpu.rip == size);
-        vcpu.rip = 0;
-        EXPECT_FALSE(memcmp(&vcpu, &vcpu_expected, sizeof(test_cpu_t)));
-        ks_free(code);
+        assemble_decode(insn, vcpu.rip, &size, &count, &ret);
+        ASSERT_TRUE(ret != EM_ERROR);
+        ret = em_emulate_insn(&em_ctxt);
+        ASSERT_TRUE(ret != EM_ERROR);
+        EXPECT_TRUE(vcpu.rip == vcpu_original.rip + size);
+        vcpu.rip = vcpu_expected.rip;
+        EXPECT_EQ(memcmp(&vcpu, &vcpu_expected, sizeof(test_cpu_t)), 0);
     }
 
     /* Test cases */
@@ -454,6 +469,16 @@ class EmulatorTest : public testing::Test {
     }
 };
 
+TEST_F(EmulatorTest, insn_unimpl) {
+    size_t size;
+    size_t count;
+    em_status_t ret = EM_CONTINUE;
+    // Opcode 0x85 (TEST r/mN, rN) is unimplemented
+    assemble_decode("test dword ptr [edx + 2*ecx + 0x10], esi", 0,
+                    &size, &count, &ret);
+    EXPECT_TRUE(ret == EM_ERROR);
+}
+
 TEST_F(EmulatorTest, insn_add) {
     test_alu_2op<8>("add", {
         { 0x04, 0x05, 0,