Inline hook handling of relative jumps not proper?

[gibbed]

safetyhook/src/inline_hook.cpp

Lines 260 to 284 in de83823

	} else if (ix.meta.category == ZYDIS_CATEGORY_COND_BR && ix.meta.branch_type == ZYDIS_BRANCH_TYPE_SHORT) {
	const auto target_address = ip + ix.length + ix.raw.imm[0].value.s;
	auto new_disp = target_address - (tramp_ip + 6);
	// Handle the case where the target is now in the trampoline.
	if (target_address < m_target + m_original_bytes.size()) {
	new_disp = static_cast<ptrdiff_t>(ix.raw.imm[0].value.s);
	}
	*tramp_ip = 0x0F;
	*(tramp_ip + 1) = 0x10 + ix.opcode;
	store(tramp_ip + 2, static_cast<int32_t>(new_disp));
	tramp_ip += 6;
	} else if (ix.meta.category == ZYDIS_CATEGORY_UNCOND_BR && ix.meta.branch_type == ZYDIS_BRANCH_TYPE_SHORT) {
	const auto target_address = ip + ix.length + ix.raw.imm[0].value.s;
	auto new_disp = target_address - (tramp_ip + 5);
	// Handle the case where the target is now in the trampoline.
	if (target_address < m_target + m_original_bytes.size()) {
	new_disp = static_cast<ptrdiff_t>(ix.raw.imm[0].value.s);
	}
	*tramp_ip = 0xE9;
	store(tramp_ip + 1, static_cast<int32_t>(new_disp));
	tramp_ip += 5;

I'm creating a MidHook at the address 00000001004751AC, which has the following instructions:

00000001004751AC  72 AE              jb  short 10047515C
00000001004751AE  48 8D BD E0F8FFFF  lea rdi, [rbp-720h]

this ends up emitting the trampoline:

00000000FFFF0327  0F82 AEFFFFFF      jb  FFFF02DB
00000000FFFF032D  48 8D BD E0F8FFFF  lea rdi, [rbp-720h]
00000000FFFF0334  E9 7C4E4800        jmp 1004751B5

The jb here did not get resolved properly (it should be a relative jmp to 000000010047515C).

target_address < m_target + m_original_bytes.size()

ends up being

000000010047515C < 00000001004751B5

So I'm guessing that check should actually be:

target_address >= m_target && target_address < m_target + m_original_bytes.size()

[praydog]

Sounds like a reasonable fix to me.

[cursey]

@gibbed can you try #87 and let me know if it actually fixes the issue you've described?

[gibbed]

Yeah, it does. Though you've missed the check for unconditional jumps.

https://github.com/cursey/safetyhook/pull/87/files#diff-b109456f39b67560bebff92603a5d7152ce3a5188182bc84c231e6c3e7019f7cL278