Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

Commit

Permalink
emulate: Abort decoding if opcode is unsupported
Browse files Browse the repository at this point in the history 
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. In em_decode_insn(), check if the opcode is unsupported, i.e.
   the INSN_NOTIMPL flag is set or there is no emulation handler.
   If so, return a fatal error, raise a vCPU panic, and log the
   raw bytes of the instruction.
2. Before decoding a new instruction, reset the emulation context,
   so the old context is not accidentally referred to.
3. Add unit tests for two unsupported opcode cases. This requires
   refactoring EmulatorTest::run() first.

Signed-off-by: Yu Ning <yu.ning@intel.com>
  • Loading branch information
@raphaelning
raphaelning committed Sep 10, 2018
1 parent a4b1abd commit 4033099
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 29 deletions.
29 changes: 18 additions & 11 deletions core/emulate.c
View file
Expand Up @@ -881,7 +881,6 @@ em_status_t EMCALL em_decode_insn(struct em_context_t *ctxt, const uint8_t *insn
uint8_t b;
uint64_t flags;
struct em_opcode_t *opcode;
const struct em_opcode_t *opcode_group;

switch (ctxt->mode) {
case EM_MODE_REAL:
Expand Down Expand Up @@ -1010,22 +1009,30 @@ em_status_t EMCALL em_decode_insn(struct em_context_t *ctxt, const uint8_t *insn
ctxt->operand_size = 1;
}
if (flags & INSN_GROUP) {
opcode_group = &opcode->group[ctxt->modrm.opc];
opcode->handler = opcode_group->handler;
if (!opcode_group->handler) {
const struct em_opcode_t *group_opcode;

if (!opcode->group) {
/* Should never happen */
return EM_ERROR;
}
opcode->flags |= opcode_group->flags;
if (opcode_group->decode_dst != decode_op_none) {
opcode->decode_dst = opcode_group->decode_dst;
group_opcode = &opcode->group[ctxt->modrm.opc];
opcode->handler = group_opcode->handler;
opcode->flags |= group_opcode->flags;
if (group_opcode->decode_dst != decode_op_none) {
opcode->decode_dst = group_opcode->decode_dst;
}
if (opcode_group->decode_src1 != decode_op_none) {
opcode->decode_src1 = opcode_group->decode_src1;
if (group_opcode->decode_src1 != decode_op_none) {
opcode->decode_src1 = group_opcode->decode_src1;
}
if (opcode_group->decode_src2 != decode_op_none) {
opcode->decode_src2 = opcode_group->decode_src2;
if (group_opcode->decode_src2 != decode_op_none) {
opcode->decode_src2 = group_opcode->decode_src2;
}
}

/* Some unimplemented opcodes have an all-zero em_opcode_t */
if ((opcode->flags & INSN_NOTIMPL) || !opcode->handler) {
return EM_ERROR;
}
return decode_operands(ctxt);
}

Expand Down
24 changes: 19 additions & 5 deletions core/vcpu.c
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1991,8 +1995,12 @@ 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]);
dump_vmcs(vcpu);
return HAX_RESUME;
}
if (em_ctxt->len != vcpu->vmx.exit_instr_length) {
Expand All @@ -2004,8 +2012,12 @@ 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]);
dump_vmcs(vcpu);
return HAX_RESUME;
}
return HAX_EXIT;
Expand Down Expand Up @@ -2156,6 +2168,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;
Expand Down
61 changes: 48 additions & 13 deletions tests/test_emulator.cpp
View file
Expand Up @@ -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 */
Expand Down Expand Up @@ -253,6 +268,16 @@ class EmulatorTest : public testing::Test {
}
}

void test_insn_unimpl(const char* insn) {
size_t size;
size_t count;
em_status_t ret = EM_CONTINUE;

assemble_decode(insn, 0, &size, &count, &ret);
// Decoding should fail
EXPECT_LT(ret, 0);
}

template <int N>
void test_insn_rN_rN(const char* insn_name,
const std::vector<test_alu_2op_t>& tests) {
Expand Down Expand Up @@ -454,6 +479,16 @@ class EmulatorTest : public testing::Test {
}
};

TEST_F(EmulatorTest, insn_unimpl_primary) {
// Opcode 0x85 (TEST r/mN, rN) is unimplemented
test_insn_unimpl("test dword ptr [edx + 2*ecx + 0x10], esi");
}

TEST_F(EmulatorTest, insn_unimpl_secondary) {
// Opcode 0xF7 /4 (MUL r/mN) is unimplemented
test_insn_unimpl("mul dword ptr [edx + 2*ecx + 0x10]");
}

TEST_F(EmulatorTest, insn_add) {
test_alu_2op<8>("add", {
{ 0x04, 0x05, 0,
Expand Down

0 comments on commit 4033099

Please sign in to comment.