zyantific / zydis

Fast and lightweight x86/x86-64 disassembler and code generation library
https://zydis.re
MIT License
3.3k stars 427 forks source link

Incorrect disassembly of push rdi #480

Closed Archie-osu closed 5 months ago

Archie-osu commented 5 months ago

Hi there, I'm having a bit of an issue with running Zydis in the Windows Kernel. I'm disassembling the HalpTimerQueryHostPerformanceCounter function on build 22631.3007 (SHA1: ec844089668811d4104e72fc7d3864caa2a37c44), but Zydis is giving incorrect results.

The issue is present on both v4.0.0 and on 15e38ac36fa4a84dde0fc6ae973d33c22e9521e9 (which I amalgamated locally). When disassembling the function, WinDbg's KD gives the correct and expected results:

0: kd> uu FFFFF804230FB7A0
nt!HalpTimerQueryHostPerformanceCounter:
fffff804`230fb7a0 48895c2408      mov     qword ptr [rsp+8],rbx
fffff804`230fb7a5 57              push    rdi
fffff804`230fb7a6 4883ec20        sub     rsp,20h
fffff804`230fb7aa 488b055f897600  mov     rax,qword ptr [nt!HalpPerformanceCounter (fffff804`23864110)]
fffff804`230fb7b1 488bf9          mov     rdi,rcx
fffff804`230fb7b4 4885c0          test    rax,rax
fffff804`230fb7b7 743f            je      nt!HalpTimerQueryHostPerformanceCounter+0x58 (fffff804`230fb7f8)
fffff804`230fb7b9 83b8e400000007  cmp     dword ptr [rax+0E4h],7

However, running Zydis in ZYDIS_MACHINE_MODE_LONG_64 gives different results:

FFFFF804230FB7A0 : mov [rsp+0x08], rbx
FFFFF804230FB7A5 : mov rax, [0xFFFFF8042386410B]
FFFFF804230FB7AC : disassembly failed
FFFFF804230FB7AD : mov eax, 0xE4
FFFFF804230FB7B2 : cmp eax, 0x766B06
FFFFF804230FB7B7 : add eax, 0x766995
FFFFF804230FB7BC : xor ecx, ecx
FFFFF804230FB7BF : dec [rax-0x75]
FFFFF804230FB7C2 : add eax, 0x766AE7
FFFFF804230FB7C7 : add rbx, rax
FFFFF804230FB7CA : xor eax, eax

My code handling Zydis looks like so:


NTSTATUS HTPDisassemble(
    IN PVOID Address, 
    IN size_t InstructionCount, 
    OUT ZydisDisassembledInstruction* Instructions
)
{
    // Convert the address into a Zydis pointer type
    ZyanUPointer runtime_address = reinterpret_cast<ZyanUPointer>(Address);

    // Our current offset in the memory_data array
    ZyanUSize offset = 0;

    // Loop until we do our instruction count
    for (size_t i = 0; i < InstructionCount; i++)
    {
        ZydisDisassembledInstruction current_instruction = { 0 };

        // We don't actually know the size here, but we assume that at least the current page is readable
        ZyanStatus disassembly_status = ZydisDisassembleIntel(
            ZYDIS_MACHINE_MODE_LONG_64,
            runtime_address,
            reinterpret_cast<PVOID>(runtime_address + offset),
            PAGE_SIZE - (runtime_address & 0xFFF),
            &current_instruction
        );

        if (!ZYAN_SUCCESS(disassembly_status))
        {
            // Just an invalid opcode, skip to the next one and continue
            if (disassembly_status == ZYDIS_STATUS_DECODING_ERROR)
            {
                DbgPrintEx(0, 0, "%016llX : disassembly failed\n", runtime_address);
                runtime_address++;
                offset++;
                continue;
            }

            // If it's another, more serious error, break out
            return STATUS_INTERNAL_ERROR;
        }

        DbgPrintEx(0, 0, "%016llX : %s\n", runtime_address, current_instruction.text);

        // Save the instruction into our buffer
        Instructions[i] = current_instruction;

        // Advance to the next one
        offset += current_instruction.info.length;
        runtime_address += current_instruction.info.length;
    }

    return STATUS_SUCCESS;
}
flobernd commented 5 months ago

Hi! Only on mobile right now, but if I'm not mistaken, your address calculation is wrong.

You add the instruction size to the runtime_address and the offset which is correct. But for the Zydis function call you pass (runtime_address + offset) as the buffer, effectively adding the instruction length twice.

Archie-osu commented 5 months ago

Oh yeah, thank you for pointing that out - seems to have been caused by some refactoring I did before implementing the rest of the code. Closing, not an actual issue.