intelxed / xed

The X86 Encoder Decoder (XED), is a software library for encoding and decoding X86 (IA32 and Intel64) instructions
https://intelxed.github.io/
Apache License 2.0
1.4k stars 145 forks source link

xed_agen() uses current instruction address in eip relative address mode #335

Open dougkwan opened 15 hours ago

dougkwan commented 15 hours ago

xed_agen() uses the address of the instruction instead of the one after the instruction in eip-relative addresses. There was a similar bug for the 64-bit case with rip. It appears that the previous bug fix missed the 32-bit case. The following prorgram reproduces the bug.


#define DEBUG 1  // Cause failed assertion to abort program.
#include <assert.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/user.h>

#include "third_party/libxed/xed-interface.h"

xed_uint64_t agen_reg_callback(xed_reg_enum_t reg, void* context,
                               xed_bool_t* error) {
  *error = 0;
  struct user_regs_struct* regs = (struct user_regs_struct*)context;
  // The only register needed for the example instruction is EIP.
  switch (reg) {
    case XED_REG_EIP:
      return (uint32_t)regs->rip;
    default:
      *error = 1;
      return 0;
  }
}

xed_uint64_t agen_segment_callback(xed_reg_enum_t reg, void* context,
                                   xed_bool_t* error) {
  // Our example instruction does not use any segment register.
  *error = 1;
  return 0;
}

int main(int argc, char** argv) {
  xed_tables_init();
  xed_agen_register_callback(agen_reg_callback, agen_segment_callback);

  // mov    0x0(%eip),%al
  const static uint8_t insn_bytes[] = {0x67, 0x8a, 0x05, 0x00,
                                       0x00, 0x00, 0x00};

  xed_decoded_inst_t insn;
  xed_decoded_inst_zero(&insn);
  xed_decoded_inst_set_mode(&insn, XED_MACHINE_MODE_LONG_64,
                            XED_ADDRESS_WIDTH_64b);
  xed_error_enum_t xed_error =
      xed_decode(&insn, insn_bytes, sizeof(insn_bytes));
  assert(xed_error == XED_ERROR_NONE);
  assert(xed_decoded_inst_valid(&insn));

  xed_uint64_t address;
  struct user_regs_struct regs;
  memset(&regs, 0, sizeof(regs));
  const uint32_t kFakeRip = 0x1000;  // Must fit in 32 bits.
  regs.rip = kFakeRip;
  xed_error = xed_agen(&insn, 0, &regs, &address);
  assert(xed_error == XED_ERROR_NONE);

  // The expected value of address 0(%eip) is kFakeRip + 7.
  uint64_t expected_address = kFakeRip + xed_decoded_inst_get_length(&insn);
  printf("address = 0x%lx, expected = 0x%lx\n", address, expected_address);

  return address == expected_address ? EXIT_SUCCESS : EXIT_FAILURE;
}

dougkwan commented 15 hours ago

I adapted the 64-bit to fix the 32-bit case and it seems to be okay:

--- a/xed/src/dec/xed-agen.c    2023-10-16 11:59:57.000000000 +0000
+++ b/xed/src/dec/xed-agen.c    2024-10-17 22:50:17.403914144 +0000
@@ -128,10 +128,17 @@
     }
     else if (addr_width == 32) {
         xed_uint32_t base32 = base_value;
-        xed_uint32_t index32 = index_value;
         xed_uint32_t disp32 = XED_STATIC_CAST(xed_uint32_t, displacement);
-        xed_uint32_t ea32 = base32  + index32 * scale + disp32;
-        xed_uint32_t lin32 = segment_base + ea32;
+        xed_uint32_t lin32 = 0;
+        if (base_reg == XED_REG_EIP) {
+            xed_uint32_t inst_len =  xed_decoded_inst_get_length(xedd);
+            lin32 = base32 + inst_len + displacement;
+        }
+        else {
+            xed_uint32_t index32 = index_value;
+            xed_uint32_t ea32 = base32  + index32 * scale + disp32;
+            lin32 = segment_base + ea32;
+        }
         out = lin32;
         // FIXME: big real mode!
     }