Fixing support for inject_x64 injection inside WOW64 processes #4989

N0fix · 2021-07-01T18:28:43Z

Those changes fixes support for inject_x64 injection on WoW64 processes, which allows support for mixed mode code, see this

Note that allocation of vmheap fails upon initializing dynamoRIO 64 on WoW64 processes. Thus, we need to pass -reachable_heap to avoid having to make this allocation.

We still need to have a proper support on drrun64 to inject natively without having to use create_process.exe.

Example command line that works :

bin64\drrun.exe -reachable_heap -inject_x64 -c .\clientdll.dll -- bin64\create_process.exe .\helloworld32.exe

As we need to specify reachable_heap I am afraid that win32.mixedmode test will be needing some tweaks.

Changes features :

Saving eax register that holds routine address for RtlUserThreadStart before mode switch, and restore it on mode switch
Fixing far jmp to switch to x64 mode on injection
Fixing env variable argument propagation

EDIT

-reachable_heap should not be required anymore since vmheap injection issues has been fixed in this commit.

derekbruening

Thank you for working on this and contributing the patch. I'm not sure I understand some of it though.

core/win32/inject.c

derekbruening · 2021-07-02T00:49:11Z

core/win32/syscall.c

+    // commenting this ASSERT since x64 client injected in
+    // WOW64 binaries will be using mc->rax as comparaison
+    // thus making the assert fail, while the syscall still
+    // being valid.


A) Who is setting the upper bits?

B) You mean the Windows kernel ignores the upper bits? So the assert is flawed?

I meant that the assert always fails due to the size of mc->rax being compared with wrong sizes, no matter if the syscall is valid or not.

I don't understand "the assert always fails due to the size of mc->rax being compared with wrong sizes"?

Trying to print the first erroneous syscall (syscall that makes the assert trigger) on WoW64 + inject_x64, I can see that mc->xax value is ffffffff00031054. I assume that this comes from a conversion somehow representing the syscall with a signed number, which outputs a value that is wrong if you display it. Since mc->xax is only used as a sysnumber and type converted to int, this is not an issue, but this still makes ASSERT_TRUNCATE (more precisely CHECK_TRUNCATE_TYPE_int) fail.

There would not be a widening conversion for an application register. App register values have to be precisely preserved. The machine context structure stores 64-bit values and prints them as such. If the top bits there are 1's, either the app set them to 1's, or somehow there is a bug where DR corrupted the register value. Is this for 32-bit app code? If it is 64-bit, we can't put it past Windows making some change to have the top bits mean something -- though in the past 64-bit code wrote to eax, zeroing the top bits.

derekbruening · 2021-07-02T00:50:20Z

core/win32/syscall.c

@@ -1744,7 +1744,10 @@ propagate_options_via_env_vars(dcontext_t *dcontext, HANDLE process_handle,
     * model without requiring setting up config files for children.
     */
    uint64 peb;
-    bool peb_is_32 = is_32bit_process(process_handle);
+    bool peb_is_32 = is_32bit_process(process_handle)
+	    // if x64 client targeting WOW64 we need to 


nit: Modern style is to prefer capitalized and punctuated whole-sentence comments.

core/win32/inject.c

core/heap.c

core/optionsx.h

core/win32/inject.c

derekbruening · 2021-07-09T03:18:24Z

All the tests are failing: looks like b/c of tabs:

ERROR: diff contains tabs:
  +	 // if x64 client targeting WOW64 we need to

The style convention is to not have any tabs.

derekbruening · 2021-07-09T16:48:26Z

Tests are all still failing with clang-format style violations:

 ERROR: diff contains trailing spaces:
  + // XXX : We should also be testing for target to be wow64 along with

-- Changes are not formatted properly:

N0fix · 2021-07-09T16:52:31Z

Tests are all still failing with clang-format style violations:

 ERROR: diff contains trailing spaces:
  + // XXX : We should also be testing for target to be wow64 along with

-- Changes are not formatted properly:

Fixed in last commit

derekbruening · 2021-07-09T17:31:50Z

There are more -- I would suggest checking the entire diff, or better following the development setup make/git/devsetup.sh https://dynamorio.org/page_workflow.html#autotoc_md296 which checks for trailing spaces and other things at local commit time.

  ERROR: diff contains trailing spaces:
  + // If x64 client targeting WOW64 we need to

There are also clang-format failures -- see the ci-clang-format test. I would suggest setting up clang-format in your editor.

N0fix · 2021-07-09T17:38:27Z

There are more -- I would suggest checking the entire diff, or better following the development setup make/git/devsetup.sh https://dynamorio.org/page_workflow.html#autotoc_md296 which checks for trailing spaces and other things at local commit time.
  ERROR: diff contains trailing spaces:
  + // If x64 client targeting WOW64 we need to
There are also clang-format failures -- see the ci-clang-format test. I would suggest setting up clang-format in your editor.

Just removed other trailing spaces.
I was looking for your dev setup, thanks for the links, I will take a look into it if this merge fails.

…ption

core/heap.c

core/win32/syscall.c

N0fix · 2021-07-13T09:41:06Z

Not sure about what is failing in Windows build. Looks like a CI error that is not coming from my code.

derekbruening · 2021-07-13T16:31:46Z

Not sure about what is failing in Windows build. Looks like a CI error that is not coming from my code.

I think it's a sourceforge server error causing doxygen to not get downloaded. Probably re-running is the first thing to try: I clicked re-run.

derekbruening · 2021-07-13T18:19:20Z

run arm tests

derekbruening · 2021-07-13T19:27:20Z

@AssadHashmi -- is there any way to enable existing contributors to trigger AArch64 Jenkins runs from a forked PR? It seems there's no way to do that now for a PR from a forked PR created by a new contributor.

derekbruening · 2021-07-13T19:29:53Z

Going to merge this w/o the Jenkins for now: the master run will catch issues. Cross-compile for these code changes gives high confidence. Once @AssadHashmi is back we'll try to figure out a better long term solution.

Fixes issues around the -inject_x64 prototype option added by PR #4653 for #803 to enable injecting a 64-bit DR into a WOW64 (32-bit) child ("mixed mode"). Xref discussion at https://groups.google.com/g/dynamorio-users/c/rhEpslerwf8 Adds a new option -vmheap_size_wow64 since the default x64 size will not fit in a WOW64 process. Saves eax register that holds routine address for RtlUserThreadStart before mode switch, and restores it on mode switch. Fixes far jmp to switch to x64 mode on injection. Fixes env variable argument propagation. Example command line that works : $ bin64\drrun.exe -inject_x64 -c .\clientdll.dll -- bin64\create_process.exe .\helloworld32.exe We still need to add proper support on drrun64 to inject natively without having to use create_process.exe. Issue: #49, #4990

AssadHashmi · 2021-07-19T17:52:44Z

add to whitelist

AssadHashmi · 2021-07-19T18:02:01Z

ok to test

AssadHashmi · 2021-07-19T18:02:20Z

test this please

derekbruening · 2021-07-19T18:36:22Z

run arm tests

derekbruening · 2021-07-19T18:36:38Z

I think once it's merged nothing will run

AssadHashmi · 2021-07-20T18:16:56Z

I think once it's merged nothing will run

You're right @derekbruening.
add to whitelist worked for a new contributor @mat-kalinowski on his first PR #5020 (comment)

N0fix · 2021-07-29T13:29:35Z

An encoding cti error is occurring inside generate_switch_mode_jmp_to_hook on latest builds (from source or cronbuild, doesn't matter), upon injecting into x86 binaries. I have been looking at commits since this PR, and I did not find something that would have interfere with this PR at first glance.

EDIT

This can be fixed changing this to :

 bool target_64 = !x86_code IF_X64(|| !DYNAMO_OPTION(inject_x64));

But this seems to create syscall issues later on on my current Win10 OS. Upon creating a message box (target app calls MessageBoxA), DynamoRIO crashes with : Usage error: Is your client invoking raw system calls via vdso sysenter? While such behavior is not recommended and can create problems, it may work with the -sysenter_is_int80 runtime option.

Windows 10 Build 19041

N0fix · 2021-07-29T23:27:16Z

I can confirm that mixed code injection is not working anymore on latest cronbuilds (18324 and 18331, since PR basically).

I have a working version of dynamoRIO with injection somewhere. I have been testing this PR again, and the error occurs. So the problem comes from my side, I might have forgotten some change I have made somewhere. I will check that soon and submit a PR fixing that.

derekbruening · 2021-07-30T01:50:04Z

A regression test should be enabled to avoid breakage.

N0fix · 2021-07-30T14:49:28Z

A regression test should be enabled to avoid breakage.

Applying modification from this PR to #4653 (#803) works perfectly well, if we add :

			if (is_32bit_process(phandle) && DYNAMO_OPTION(inject_x64)) {
				target_64 = false;
			}

that I did forget in my PR.

The syscall trouble must be coming from some other commit since this PR. Would you know which commit might have mess with syscall handling since then?

N0fix · 2021-07-30T17:23:38Z

Trouble starts from cronbuild-8.0.18663, we need to check commits between 18655 and 18663.

N0fix added 2 commits July 1, 2021 18:44

Fixing x64 -> WOW64 injection using x64 client (inject_x64 option)

4fa5ee8

Fixing x64 -> WOW64 injection using x64 client (inject_x64 option)

9543dde

N0fix mentioned this pull request Jul 1, 2021

CRASH injecting x64 in WOW64 #4990

Open

N0fix changed the title ~~[PR] Fixing support for inject_x64 injection inside WOW64 processes~~ Fixing support for inject_x64 injection inside WOW64 processes Jul 1, 2021

derekbruening reviewed Jul 2, 2021

View reviewed changes

derekbruening reviewed Jul 3, 2021

View reviewed changes

core/win32/inject.c Show resolved Hide resolved

N0fix added 5 commits July 7, 2021 13:21

Fixing clang CI format + adding option for vmheap size on wow64

898b8c2

Removing old draft code leftover

e4500c3

fixing offset in comment

24e27d7

Merge branch 'master' into master

987776f

Fixing formating in heap.c

7a6cac7

derekbruening reviewed Jul 9, 2021

View reviewed changes

core/heap.c Outdated Show resolved Hide resolved

core/optionsx.h Show resolved Hide resolved

core/win32/inject.c Show resolved Hide resolved

Fixing tabs and adding comments

813c900

Removing trailing spaces...

d7b0a9f

Merge branch 'master' into master

32523c0

N0fix added 2 commits July 9, 2021 19:33

Removing trailing spaces... again, in syscall.c

246b9e5

Merge branch 'master' of https://github.com/N0fix/dynamorio

9c2eaed

N0fix added 4 commits July 9, 2021 21:05

Fixing line len heap.c

f687060

Fixed heap.c + adding vmheap size that depends on wow64 + injectx64 o…

b5b9ee6

…ption

Fixed (again, wtf) len of comments in heap.c

382c05d

Removing trailing white spaces obviously

efad60a

derekbruening approved these changes Jul 12, 2021

View reviewed changes

core/heap.c Outdated Show resolved Hide resolved

core/win32/syscall.c Outdated Show resolved Hide resolved

N0fix added 3 commits July 12, 2021 18:22

Fixing x86 build + added ref to #49 in syscall and reenabling ASSERT

dd5ec76

Used global defines to manage heap size

9cd5bcc

Fixing macro

ef793cc

N0fix added 4 commits July 13, 2021 10:52

Merge branch 'master' into master

ff4fc71

clang format

b2c95d8

Merge branch 'master' of https://github.com/N0fix/dynamorio

24e7ca0

clang format

a4e2612

derekbruening merged commit 89841a1 into DynamoRIO:master Jul 13, 2021

N0fix added a commit to N0fix/dynamorio that referenced this pull request Aug 2, 2021

Fixing PR DynamoRIO#4989

ad91661

N0fix mentioned this pull request Aug 2, 2021

i#4990: Cross injection x64 to x86 failing (#803) #5032

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing support for inject_x64 injection inside WOW64 processes #4989

Fixing support for inject_x64 injection inside WOW64 processes #4989

N0fix commented Jul 1, 2021 •

edited

Loading

derekbruening left a comment

derekbruening Jul 2, 2021

N0fix Jul 7, 2021

derekbruening Jul 8, 2021

N0fix Jul 9, 2021 •

edited

Loading

derekbruening Jul 9, 2021

derekbruening Jul 2, 2021

derekbruening commented Jul 9, 2021

derekbruening commented Jul 9, 2021

N0fix commented Jul 9, 2021

derekbruening commented Jul 9, 2021

N0fix commented Jul 9, 2021

N0fix commented Jul 13, 2021

derekbruening commented Jul 13, 2021

derekbruening commented Jul 13, 2021

derekbruening commented Jul 13, 2021

derekbruening commented Jul 13, 2021

AssadHashmi commented Jul 19, 2021

AssadHashmi commented Jul 19, 2021

AssadHashmi commented Jul 19, 2021

derekbruening commented Jul 19, 2021

derekbruening commented Jul 19, 2021

AssadHashmi commented Jul 20, 2021

N0fix commented Jul 29, 2021 •

edited

Loading

N0fix commented Jul 29, 2021 •

edited

Loading

derekbruening commented Jul 30, 2021

N0fix commented Jul 30, 2021 •

edited

Loading

N0fix commented Jul 30, 2021

Fixing support for inject_x64 injection inside WOW64 processes #4989

Fixing support for inject_x64 injection inside WOW64 processes #4989

Conversation

N0fix commented Jul 1, 2021 • edited Loading

EDIT

derekbruening left a comment

Choose a reason for hiding this comment

derekbruening Jul 2, 2021

Choose a reason for hiding this comment

N0fix Jul 7, 2021

Choose a reason for hiding this comment

derekbruening Jul 8, 2021

Choose a reason for hiding this comment

N0fix Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

derekbruening Jul 9, 2021

Choose a reason for hiding this comment

derekbruening Jul 2, 2021

Choose a reason for hiding this comment

derekbruening commented Jul 9, 2021

derekbruening commented Jul 9, 2021

N0fix commented Jul 9, 2021

derekbruening commented Jul 9, 2021

N0fix commented Jul 9, 2021

N0fix commented Jul 13, 2021

derekbruening commented Jul 13, 2021

derekbruening commented Jul 13, 2021

derekbruening commented Jul 13, 2021

derekbruening commented Jul 13, 2021

AssadHashmi commented Jul 19, 2021

AssadHashmi commented Jul 19, 2021

AssadHashmi commented Jul 19, 2021

derekbruening commented Jul 19, 2021

derekbruening commented Jul 19, 2021

AssadHashmi commented Jul 20, 2021

N0fix commented Jul 29, 2021 • edited Loading

EDIT

N0fix commented Jul 29, 2021 • edited Loading

derekbruening commented Jul 30, 2021

N0fix commented Jul 30, 2021 • edited Loading

N0fix commented Jul 30, 2021

N0fix commented Jul 1, 2021 •

edited

Loading

N0fix Jul 9, 2021 •

edited

Loading

N0fix commented Jul 29, 2021 •

edited

Loading

N0fix commented Jul 29, 2021 •

edited

Loading

N0fix commented Jul 30, 2021 •

edited

Loading