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

Alignment issues with instructions that use 128bit registers and data #8

Open
avncharlie opened this issue Jul 29, 2023 · 4 comments
Open

Comments

@avncharlie
Copy link

Hello,

I ran into an issue inserting a patch with data into a binary that uses instructions operating on 128bit registers, specifically this instruction: xorpd xmm2, XMMWORD PTR [rip+0x69aed].
This was as the memory operand needed to be 16 byte aligned and the data I had inserted with my patch had moved this alignment. After I ensured my patch was 16-byte aligned the binary rewrote successfully.
I'm not sure if this is actually an issue or maybe more something to be aware of when writing patches but I thought I might make an issue for anyone else with this problem.

@jranieri-grammatech
Copy link
Collaborator

It depends on what exactly your patch was doing. Could you post an example transform that reproduces this?

@avncharlie
Copy link
Author

avncharlie commented Aug 2, 2023

Sure, here's a minimal example.

Below is the c program used.

#include <stdio.h>
#include <immintrin.h> 

int main() {
    __m128d a = _mm_set_pd(5.0, 3.0);
    __m128d b = _mm_set_pd(2.5, 4.5);
    __m128d result;

    result = _mm_and_pd(a, b);

    printf("Result: %lf %lf\n", result[1], result[0]);
    return 0;
}

To compile: gcc -O1 -o test test.c
My gcc version is gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0

Using -01 causes the compiler to generate instructions that rely on 16 byte aligned data, namely movapd and andpd in this example.

Here is the patch:

context.register_insert_function(
    'test_alignment_func',
    Patch.from_function(lambda _: '''
        nop

        .rodata
        .TEST_STRING:
            .string "TEST STRING"
    ''', Constraints(x86_syntax=X86Syntax.INTEL))
)

When running the original program:

$ ./test
Result: 2.500000 2.000000

When running the instrumented program:

$ ./test_instrumented
[1]    229030 segmentation fault  ./test_instrumented

Running under gdb shows the instrumented program crashed while attempting to call movapd on a memory address that isn't 16 byte aligned:
image

Here is a comparison of the read only memory areas of the original and instrumented binary (dumped using gdb), it appears that the string has been added at the start of this memory area and misaligned everything after it:
image

Adding some space to the patch to ensure the added data is 16 byte aligned fixes the issue:

context.register_insert_function(
    'test_alignment_func',
    Patch.from_function(lambda _: '''
    nop

    .rodata
    .TEST_STRING:
        .string "TEST STRING"
        .space 4
    ''', Constraints(x86_syntax=X86Syntax.INTEL))
)
$ ./test_instrumented
Result: 2.500000 2.000000

@jranieri-grammatech
Copy link
Collaborator

jranieri-grammatech commented Aug 14, 2023

I've reproduced this and it probably could be considered a bug in ddisasm. Until that gets addressed, a potential workaround is to:

  • disassemble all code blocks in the module using GtirbInstructionDecoder
  • use the resulting capstone instructions to determine if there is a 16-byte or larger memory access (a proxy for being an SSE/AVX instruction)
  • examine the GTIRB symbolic expression corresponding with that memory access
  • mark the referenced block as needing 16-byte alignment in the 'alignment' aux data table

(or some other variation of using the instructions to infer which data blocks must be 16-byte aligned)

@avncharlie
Copy link
Author

For anyone else coming across this, binaries built with MSVC will crash in initialisation code if data added by gtirb-rewriting isn't aligned.

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

No branches or pull requests

2 participants