Fix ThumbRelocator IT block eoi logic (#430)

Instructions inside the block should not result in eoi.
frida · May 18, 2020 · 6f4a8f5 · 6f4a8f5
1 parent 21e1448
commit 6f4a8f5
Show file tree

Hide file tree

Showing 2 changed files with 97 additions and 36 deletions.
diff --git a/gum/arch-arm/gumthumbrelocator.c b/gum/arch-arm/gumthumbrelocator.c
@@ -214,43 +214,46 @@ gum_thumb_relocator_read_one (GumThumbRelocator * self,
   if (!cs_disasm_iter (self->capstone, &code, &size, &address, insn))
     return 0;
 
-  switch (insn->id)
+  if (!self->it_block.active)
   {
-    case ARM_INS_B:
-    case ARM_INS_BX:
-      self->eob = TRUE;
-      self->eoi = gum_arm_branch_is_unconditional (insn);
-      break;
-    case ARM_INS_CBZ:
-    case ARM_INS_CBNZ:
-    case ARM_INS_BL:
-    case ARM_INS_BLX:
-      self->eob = TRUE;
-      self->eoi = FALSE;
-      break;
-    case ARM_INS_LDR:
-      self->eob = self->eoi = gum_reg_dest_is_pc (insn);
-      break;
-    case ARM_INS_POP:
-      self->eob = self->eoi = gum_reg_list_contains_pc (insn, 0);
-      break;
-    case ARM_INS_LDM:
-      self->eob = self->eoi = gum_reg_list_contains_pc (insn, 1);
-      break;
-    case ARM_INS_IT:
+    switch (insn->id)
     {
-      it_block_size = gum_parse_it_instruction_block_size (
-          GUINT16_FROM_LE (*((guint16 *) self->input_cur)));
-      self->eob = FALSE;
-      break;
+      case ARM_INS_B:
+      case ARM_INS_BX:
+        self->eob = TRUE;
+        self->eoi = gum_arm_branch_is_unconditional (insn);
+        break;
+      case ARM_INS_CBZ:
+      case ARM_INS_CBNZ:
+      case ARM_INS_BL:
+      case ARM_INS_BLX:
+        self->eob = TRUE;
+        self->eoi = FALSE;
+        break;
+      case ARM_INS_LDR:
+        self->eob = self->eoi = gum_reg_dest_is_pc (insn);
+        break;
+      case ARM_INS_POP:
+        self->eob = self->eoi = gum_reg_list_contains_pc (insn, 0);
+        break;
+      case ARM_INS_LDM:
+        self->eob = self->eoi = gum_reg_list_contains_pc (insn, 1);
+        break;
+      case ARM_INS_IT:
+      {
+        it_block_size = gum_parse_it_instruction_block_size (
+            GUINT16_FROM_LE (*((guint16 *) self->input_cur)));
+        self->eob = TRUE;
+        break;
+      }
+      case ARM_INS_TBB:
+      case ARM_INS_TBH:
+        self->eob = self->eoi = TRUE;
+        break;
+      default:
+        self->eob = FALSE;
+        break;
     }
-    case ARM_INS_TBB:
-    case ARM_INS_TBH:
-      self->eob = self->eoi = TRUE;
-      break;
-    default:
-      self->eob = FALSE;
-      break;
   }
 
   gum_thumb_relocator_increment_inpos (self);
@@ -261,8 +264,15 @@ gum_thumb_relocator_read_one (GumThumbRelocator * self,
   self->input_cur += insn->size;
   self->input_pc += insn->size;
 
-  while (it_block_size--)
-    gum_thumb_relocator_read_one (self, NULL);
+  if (it_block_size > 0)
+  {
+    self->it_block.active = TRUE;
+
+    while (it_block_size--)
+      gum_thumb_relocator_read_one (self, NULL);
+
+    self->it_block.active = FALSE;
+  }
 
   return self->input_cur - input_start;
 }

diff --git a/tests/core/arch-arm/thumbrelocator.c b/tests/core/arch-arm/thumbrelocator.c
@@ -30,6 +30,7 @@ TESTLIST_BEGIN (thumbrelocator)
   TESTENTRY (it_block_with_pc_relative_load_should_be_rewritten)
   TESTENTRY (it_block_with_b_should_be_rewritten)
   TESTENTRY (it_block_should_be_rewritten_as_a_whole)
+  TESTENTRY (it_block_with_eoi_insn_should_be_rewritten)
   TESTENTRY (eob_and_eoi_on_ret)
 TESTLIST_END ()
 
@@ -760,6 +761,56 @@ TESTCASE (it_block_should_be_rewritten_as_a_whole)
       sizeof (expected_output));
 }
 
+TESTCASE (it_block_with_eoi_insn_should_be_rewritten)
+{
+  const guint16 input[] = {
+    GUINT16_TO_LE (0x2800),                         /* cmp r0, #0     */
+    GUINT16_TO_LE (0xbf18),                         /* it ne          */
+    GUINT16_TO_LE (0xe8bd), GUINT16_TO_LE (0x8010), /* pop.w {r4, pc} */
+    GUINT16_TO_LE (0x3001),                         /* adds r0, #1    */
+  };
+  const guint16 expected_output[] = {
+    GUINT16_TO_LE (0x2800),                         /* cmp r0, #0     */
+    GUINT16_TO_LE (0xd100),                         /* bne.n if_true  */
+    GUINT16_TO_LE (0xe001),                         /* b.n if_false   */
+    /* if_true:                                                       */
+    GUINT16_TO_LE (0xe8bd), GUINT16_TO_LE (0x8010), /* pop.w {r4, pc} */
+    /* if_false:                                                      */
+    GUINT16_TO_LE (0x3001),                         /* adds r0, #1    */
+  };
+  const cs_insn * insn;
+
+  SETUP_RELOCATOR_WITH (input);
+
+  insn = NULL;
+  g_assert_cmpuint (gum_thumb_relocator_read_one (&fixture->rl, &insn), ==, 2);
+  g_assert_cmpint (insn->id, ==, ARM_INS_CMP);
+  assert_outbuf_still_zeroed_from_offset (0);
+
+  g_assert_false (fixture->rl.eob);
+
+  insn = NULL;
+  g_assert_cmpuint (gum_thumb_relocator_read_one (&fixture->rl, &insn), ==, 8);
+  g_assert_cmpint (insn->id, ==, ARM_INS_IT);
+  assert_outbuf_still_zeroed_from_offset (0);
+
+  g_assert_true (fixture->rl.eob);
+
+  insn = NULL;
+  g_assert_cmpuint (gum_thumb_relocator_read_one (&fixture->rl, &insn), ==, 10);
+  g_assert_cmpint (insn->id, ==, ARM_INS_ADD);
+  assert_outbuf_still_zeroed_from_offset (0);
+
+  g_assert_true (gum_thumb_relocator_write_one (&fixture->rl));
+  g_assert_true (gum_thumb_relocator_write_one (&fixture->rl));
+  g_assert_true (gum_thumb_relocator_write_one (&fixture->rl));
+  g_assert_true (gum_thumb_relocator_write_one (&fixture->rl));
+  g_assert_false (gum_thumb_relocator_write_one (&fixture->rl));
+
+  check_output (input, sizeof (input), fixture->output, expected_output,
+      sizeof (expected_output));
+}
+
 TESTCASE (eob_and_eoi_on_ret)
 {
   const guint16 input[] = {