Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sleigh to 10.2.3 #160

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Update sleigh to 10.2.3 #160

wants to merge 15 commits into from

Conversation

ekilmer
Copy link
Collaborator

@ekilmer ekilmer commented Nov 16, 2022

Contains some other small fixes/refactor.

  • Confirm whether the failing test is a new bug or was always a bug
    • There is a new bug in this PR
  • Find cause of bug from this PR

Comment on lines +299 to +300
AttributeId::initialize();
ElementId::initialize();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required with 10.2.

We had to make similar changes here lifting-bits/sleigh@f38eee4#diff-aed2f30b604fcd3c832b0255d6cd0ef5a7cca4cab0d8d2c77ee6d125d2fcf301

@ekilmer
Copy link
Collaborator Author

ekilmer commented Nov 16, 2022

Hmmmm. Seems we need to change some compiler flags somewhere https://github.com/trailofbits/maat/actions/runs/3476336902/jobs/5811473141#step:5:21433

I think it's using the pre-built sleigh libraries, so I can try looking into this on lifting-bits/sleigh

[ 97%] Linking CXX shared module maat.cpython-38-x86_64-linux-gnu.so
/usr/bin/ld: /usr/local/lib/libsla.a(xml.cc.o): relocation R_X86_64_PC32 against symbol `_ZN11TreeHandler19ignorableWhitespaceEPKcii' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [CMakeFiles/maat_python.dir/build.make:1189: maat.cpython-38-x86_64-linux-gnu.so] Error 1

@ekilmer ekilmer force-pushed the ekilmer/update-sleigh branch from 086990c to 5b8a674 Compare November 16, 2022 18:22
@ekilmer ekilmer changed the title Update sleigh to 10.2.1 Update sleigh to 10.2.2 Nov 16, 2022
@ekilmer
Copy link
Collaborator Author

ekilmer commented Nov 16, 2022

Fixed the sleigh PIC library (and also updated to 10.2.2).

Now a test is failing https://github.com/trailofbits/maat/actions/runs/3482037438/jobs/5823881087#step:10:24

[+] Testing arch X86 support...      
Fail: ArchX86: failed to disassembly and/or execute PUNPCKHDQ

We used the wrong `python3` command compared to what CMake found
@ekilmer
Copy link
Collaborator Author

ekilmer commented Dec 22, 2022

Digging more into this... Since there were multiple assertions with the same message, I added something to differentiate them (I can commit this if you want), and the specific test that is failing is here.

code = string("\x0F\x6A\x00", 3); // punpckhdq mm0, [eax]
sym.mem->write_buffer(0x1050, (uint8_t*)code.c_str(), code.size());
sym.mem->write_buffer(0x1050+code.size(), (uint8_t*)string("\xeb\x0e", 2).c_str(), 2);
sym.cpu.ctx().set(X86::MM0, exprcst(64, 0xdeadbeef12121212));
sym.cpu.ctx().set(X86::EAX, exprcst(32, 0x1900));
sym.mem->write(0x1900, 0xab001200abababab, 8);
sym.run_from(0x1050, 1);
nb += _assert( sym.cpu.ctx().get(X86::MM0).as_uint() == 0xababababdeadbeef, "ArchX86: failed to disassembly and/or execute PUNPCKHDQ");

I still need to debug to determine the actual value that is causing the failure, though.

I can also run and pass all tests on master branch, so it's something weird with the upgrade between Sleigh 10.1.2 to 10.2.2. I'll do some testing

@@ -6388,7 +6388,7 @@ namespace test
sym.cpu.ctx().set(X86::EAX, exprcst(32, 0x1900));
sym.mem->write(0x1900, 0xab001200abababab, 8);
sym.run_from(0x1050, 1);
nb += _assert( sym.cpu.ctx().get(X86::MM0).as_uint() == 0xababababdeadbeef, "ArchX86: failed to disassembly and/or execute PUNPCKHDQ");
nb += _assert( sym.cpu.ctx().get(X86::MM0).as_uint() == 0xab001200deadbeef, "ArchX86: failed to disassembly and/or execute PUNPCKHDQ");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running with a debugger, I see that the failing test has values as follows:

actual:   0xab001200deadbeef
expected: 0xababababdeadbeef

I pushed the change (to see if CI passes), but I'm not sure if this was a bug in the last implementation or a bug now 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on this: Using a microx script (with a fix), I can confirm that the original value expected (0xababababdeadbeef) is correct, and there is likely a bug in sleigh somehow.

Click to see `microx` script
import traceback
import microx

def main():
    o = microx.Operations()

    code = microx.ArrayMemoryMap(o, 0x1000, 0x2000, can_write=False, can_execute=True)
    stack = microx.ArrayMemoryMap(o, 0x80000, 0x82000)
    heap = microx.ArrayMemoryMap(o, 0x10000, 0x12000)

    code.store_bytes(
        0x1050,
        # punpckhdq mm0, [eax]
        b"\x0F\x6A\x00\xeb\x0e"
    )
    heap.store_bytes(
        0x10900,
        b"\xab\x00\x12\x00\xab\xab\xab\xab"
    )

    m = microx.Memory(o, 32)
    m.add_map(code)
    m.add_map(stack)
    m.add_map(heap)

    t = microx.EmptyThread(o)
    t.write_register("EIP", 0x1050)
    t.write_register("ESP", 0x81000)

    t.write_register("MM0", 0xdeadbeef12121212)
    t.write_register("EAX", 0x10900)

    p = microx.Process(o, m)

    try:
        pc = t.read_register("EIP", t.REG_HINT_PROGRAM_COUNTER)
        eax = t.read_register("EAX", t.REG_HINT_MEMORY_BASE_ADDRESS)
        mm0 = t.read_register("MM0", t.REG_HINT_GENERAL)
        print(
            f"Emulating instruction at {pc:08x} (EAX={eax:08x}, MM0={mm0:08x}"
        )
        p.execute(t, 1)
        pc = t.read_register("EIP", t.REG_HINT_PROGRAM_COUNTER)
        eax = t.read_register("EAX", t.REG_HINT_MEMORY_BASE_ADDRESS)
        mm0 = t.read_register("MM0", t.REG_HINT_GENERAL)
        print(
            f"Finished emulating instruction: (EAX={eax:08x}, MM0={mm0:08x}"
        )
    except Exception as e:
        print(e)
        print(traceback.format_exc())

if __name__ == "__main__":
    main()

Not sure if this value is correct or not but at least it shows what the
value actually is for further debugging without reproducing
Seems that Ghidra has fixed some bugs for these instructions.

I tested other instructions that are commented but they still fail
@ekilmer ekilmer force-pushed the ekilmer/update-sleigh branch from dfb8f62 to 5c03f5c Compare December 29, 2022 20:45
@ekilmer ekilmer marked this pull request as ready for review December 30, 2022 13:59
@ekilmer ekilmer marked this pull request as draft January 8, 2023 17:32
*Not sure if these are true fixes. Found while investigating sleigh
regression
Future versions of sleigh will remove 'using namespace std;' from
headers
Update language spec files
@ekilmer ekilmer changed the title Update sleigh to 10.2.2 Update sleigh to 10.2.3 Mar 30, 2023
ekilmer added 4 commits March 30, 2023 10:23
Upgrading Z3 causes test failures due to incomplete model for all
expected variables. Previous versions reported values for all variables,
but now tests should check whether a value has been assigned before
testing the value.
@ekilmer
Copy link
Collaborator Author

ekilmer commented Apr 2, 2023

Another update: The bug seems to be an internal sleigh issue because replacing the new x86.sla file with the old working one still fails the test.

@ekilmer
Copy link
Collaborator Author

ekilmer commented Apr 2, 2023

I confirmed that the PUNPCKHDQ bug appears between 10.1.5 and 10.2. There were quite a few big changes between these versions.

Bisecting might be made a little easier by using sleigh-cmake HEAD release type between 10.1.5 and 10.2 at the bot update commits lifting-bits/sleigh@v10.1.5...v10.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant