From 9a3e342000f3261b5dee6475d7cf755712584643 Mon Sep 17 00:00:00 2001 From: Alexandro Sanchez Bach Date: Wed, 21 Nov 2018 14:58:51 +0100 Subject: [PATCH] Fixed out-of-bounds behavior for btx mN,rN instructions The bit-test instructions BT/BTC/BTR/BTS, through the variant `bt* mN,rN`, allow selecting bytes outside the value referenced by the memory operand. This is now checked for all instructions flagged with INSN_BITOP. Corresponding tests have been added as well. See also: Intel SDM Vol. 2A: Figure 3-2. Memory Bit Indexing. Additionally, the test_read_memory and test_write_memory functions have dropped the `ea &= 0xFF` line since to prevent subtle bugs related to memory addressing. The correctness of the memory address will be verified. Signed-off-by: Alexandro Sanchez Bach --- core/emulate.c | 70 +++++++++++++++++++++++++++++------- tests/test_emulator.cpp | 80 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 134 insertions(+), 16 deletions(-) diff --git a/core/emulate.c b/core/emulate.c index e19a5e7d..3b3d07b4 100644 --- a/core/emulate.c +++ b/core/emulate.c @@ -47,6 +47,8 @@ #define INSN_NOFLAGS ((uint64_t)1 << 6) /* Instruction has two memory operands */ #define INSN_TWOMEM ((uint64_t)1 << 7) +/* Instruction takes bit test operands */ +#define INSN_BITOP ((uint64_t)1 << 8) /* String instruction */ #define INSN_STRING (INSN_REP|INSN_REPX) @@ -246,19 +248,19 @@ static const struct em_opcode_t opcode_table_0F[256] = { X16(N), X16(N), /* 0xA0 - 0xAF */ X3(N), - F(em_bt, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM), + F(em_bt, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM | INSN_BITOP), X7(N), - F(em_bts, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM), + F(em_bts, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM | INSN_BITOP), X4(N), /* 0xB0 - 0xBF */ X3(N), - F(em_btr, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM), + F(em_btr, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM | INSN_BITOP), X2(N), I(em_movzx, op_modrm_reg, op_modrm_rm8, op_none, INSN_MODRM | INSN_MOV), I(em_movzx, op_modrm_reg, op_modrm_rm16, op_none, INSN_MODRM | INSN_MOV), X2(N), - G(opcode_group8, op_modrm_rm, op_simm8, op_none, 0), - F(em_btc, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM), + G(opcode_group8, op_modrm_rm, op_simm8, op_none, INSN_BITOP), + F(em_btc, op_modrm_rm, op_modrm_reg, op_none, INSN_MODRM | INSN_BITOP), X2(N), I(em_movsx, op_modrm_reg, op_modrm_rm8, op_none, INSN_MODRM | INSN_MOV), I(em_movsx, op_modrm_reg, op_modrm_rm16, op_none, INSN_MODRM | INSN_MOV), @@ -382,6 +384,41 @@ static em_status_t segmented_write(struct em_context_t *ctxt, return ctxt->ops->write_memory(ctxt->vcpu, la, data, size, flags); } +static em_status_t operand_read_bitop(struct em_context_t *ctxt) +{ + em_status_t rc; + struct em_operand_t *base = &ctxt->dst; + struct em_operand_t *offs = &ctxt->src1; + int64_t offset; + + if (base->flags & OP_READ_PENDING) { + rc = ctxt->ops->read_memory_post(ctxt->vcpu, &base->value, base->size); + return rc; + } + + offset = READ_GPR(offs->reg.index, offs->size); + switch (offs->size) { + case 2: + offset = (int16_t)offset; + break; + case 4: + offset = (int32_t)offset; + break; + case 8: + offset = (int64_t)offset; + break; + default: + return EM_ERROR; + } + base->mem.ea += (offset >> 3); + offs->value = (offset & 0x7); + rc = segmented_read(ctxt, &base->mem, &base->value, base->size); + if (rc != EM_CONTINUE) { + base->flags |= OP_READ_PENDING; + } + return rc; +} + static em_status_t operand_read(struct em_context_t *ctxt, struct em_operand_t *op) { @@ -1072,17 +1109,24 @@ em_status_t EMCALL em_emulate_insn(struct em_context_t *ctxt) if (!(opcode->flags & INSN_NOFLAGS)) { ctxt->rflags = ctxt->ops->read_rflags(ctxt->vcpu); } - if (!(opcode->flags & INSN_MOV)) { - rc = operand_read(ctxt, &ctxt->dst); + if (opcode->flags & INSN_BITOP && + ctxt->dst.type == OP_MEM && ctxt->src1.type == OP_REG) { + rc = operand_read_bitop(ctxt); + if (rc != EM_CONTINUE) + goto exit; + } else { + if (!(opcode->flags & INSN_MOV)) { + rc = operand_read(ctxt, &ctxt->dst); + if (rc != EM_CONTINUE) + goto exit; + } + rc = operand_read(ctxt, &ctxt->src1); + if (rc != EM_CONTINUE) + goto exit; + rc = operand_read(ctxt, &ctxt->src2); if (rc != EM_CONTINUE) goto exit; } - rc = operand_read(ctxt, &ctxt->src1); - if (rc != EM_CONTINUE) - goto exit; - rc = operand_read(ctxt, &ctxt->src2); - if (rc != EM_CONTINUE) - goto exit; // Emulate instruction if (opcode->flags & INSN_FASTOP) { diff --git a/tests/test_emulator.cpp b/tests/test_emulator.cpp index d78ed4ba..202343b9 100644 --- a/tests/test_emulator.cpp +++ b/tests/test_emulator.cpp @@ -97,7 +97,6 @@ void test_advance_rip(void* obj, uint64_t len) { em_status_t test_read_memory(void* obj, uint64_t ea, uint64_t* value, uint32_t size, uint32_t flags) { test_cpu_t* vcpu = reinterpret_cast(obj); - ea &= 0xFF; if (ea + size >= 0x100) { return EM_ERROR; } @@ -123,7 +122,6 @@ em_status_t test_read_memory(void* obj, uint64_t ea, uint64_t* value, em_status_t test_write_memory(void* obj, uint64_t ea, uint64_t* value, uint32_t size, uint32_t flags) { test_cpu_t* vcpu = reinterpret_cast(obj); - ea &= 0xFF; if (ea + size > 0x100) { return EM_ERROR; } @@ -495,10 +493,10 @@ class EmulatorTest : public testing::Test { if (N == 64 && sizeof(void*) < 8) { return; } + // Only bit-test variants that implicitly use bit offset modulo width. test_insn_rN_rM(insn_name, tests); test_insn_rN_iM(insn_name, tests); test_insn_mN_iM(insn_name, tests); - test_insn_mN_rM(insn_name, tests); } template @@ -629,6 +627,24 @@ TEST_F(EmulatorTest, insn_bt) { { 0x80000000'00000000ULL, 0xFF, 0, 0x80000000'00000000ULL, RFLAGS_CF }, }); + + // Variant `bt mN,rN`: Modifies the EA based on the bit offset. + test_cpu_t vcpu_original; + test_cpu_t vcpu_expected; + vcpu_original = {}; + vcpu_original.gpr[REG_RCX] = 0ULL; + (uint64_t&)vcpu_original.mem[0x00] = 0x00020000'00000000; + (uint64_t&)vcpu_original.mem[0x08] = 0x00000000'00000000; + (uint64_t&)vcpu_original.mem[0x10] = 0xFFFFFFFF'FFFFFFFE; + + vcpu_original.gpr[REG_RAX] = -15LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags |= RFLAGS_CF; + run("bt [ecx + 0x08], eax", vcpu_original, vcpu_expected); + vcpu_original.gpr[REG_RAX] = +64LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags &= ~RFLAGS_CF; + run("bt [rcx + 0x08], rax", vcpu_original, vcpu_expected); } TEST_F(EmulatorTest, insn_btc) { @@ -650,6 +666,26 @@ TEST_F(EmulatorTest, insn_btc) { { 0x80000000'00000000ULL, 0xFF, 0, 0x00000000'00000000ULL, RFLAGS_CF }, }); + + // Variant `btc mN,rN`: Modifies the EA based on the bit offset. + test_cpu_t vcpu_original; + test_cpu_t vcpu_expected; + vcpu_original = {}; + vcpu_original.gpr[REG_RCX] = 0ULL; + (uint64_t&)vcpu_original.mem[0x00] = 0x00020000'00000000; + (uint64_t&)vcpu_original.mem[0x08] = 0x00000000'00000000; + (uint64_t&)vcpu_original.mem[0x10] = 0xFFFFFFFF'FFFFFFFE; + + vcpu_original.gpr[REG_RAX] = -15LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags |= RFLAGS_CF; + (uint64_t&)vcpu_expected.mem[0x00] = 0x00000000'00000000; + run("btc [ecx + 0x08], eax", vcpu_original, vcpu_expected); + vcpu_original.gpr[REG_RAX] = +64LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags &= ~RFLAGS_CF; + (uint64_t&)vcpu_expected.mem[0x10] = 0xFFFFFFFF'FFFFFFFF; + run("btc [rcx + 0x08], rax", vcpu_original, vcpu_expected); } TEST_F(EmulatorTest, insn_btr) { @@ -671,6 +707,25 @@ TEST_F(EmulatorTest, insn_btr) { { 0x80000000'00000000ULL, 0xFF, 0, 0x00000000'00000000ULL, RFLAGS_CF }, }); + + // Variant `btr mN,rN`: Modifies the EA based on the bit offset. + test_cpu_t vcpu_original; + test_cpu_t vcpu_expected; + vcpu_original = {}; + vcpu_original.gpr[REG_RCX] = 0ULL; + (uint64_t&)vcpu_original.mem[0x00] = 0x00020000'00000000; + (uint64_t&)vcpu_original.mem[0x08] = 0x00000000'00000000; + (uint64_t&)vcpu_original.mem[0x10] = 0xFFFFFFFF'FFFFFFFE; + + vcpu_original.gpr[REG_RAX] = -15LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags |= RFLAGS_CF; + (uint64_t&)vcpu_expected.mem[0x00] = 0x00000000'00000000; + run("btr [ecx + 0x08], eax", vcpu_original, vcpu_expected); + vcpu_original.gpr[REG_RAX] = +64LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags &= ~RFLAGS_CF; + run("btr [rcx + 0x08], rax", vcpu_original, vcpu_expected); } TEST_F(EmulatorTest, insn_bts) { @@ -692,6 +747,25 @@ TEST_F(EmulatorTest, insn_bts) { { 0x80000000'00000000ULL, 0xFF, 0, 0x80000000'00000000ULL, RFLAGS_CF }, }); + + // Variant `bts mN,rN`: Modifies the EA based on the bit offset. + test_cpu_t vcpu_original; + test_cpu_t vcpu_expected; + vcpu_original = {}; + vcpu_original.gpr[REG_RCX] = 0ULL; + (uint64_t&)vcpu_original.mem[0x00] = 0x00020000'00000000; + (uint64_t&)vcpu_original.mem[0x08] = 0x00000000'00000000; + (uint64_t&)vcpu_original.mem[0x10] = 0xFFFFFFFF'FFFFFFFE; + + vcpu_original.gpr[REG_RAX] = -15LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags |= RFLAGS_CF; + run("bts [ecx + 0x08], eax", vcpu_original, vcpu_expected); + vcpu_original.gpr[REG_RAX] = +64LL; + vcpu_expected = vcpu_original; + vcpu_expected.flags &= ~RFLAGS_CF; + (uint64_t&)vcpu_expected.mem[0x10] = 0xFFFFFFFF'FFFFFFFF; + run("bts [rcx + 0x08], rax", vcpu_original, vcpu_expected); } TEST_F(EmulatorTest, insn_movs) {