[PATCH v4 0/2] MR5222: kernelbase: Allow WriteProcessMemory to succeed on PAGE_EXECUTE_READ.
More details here: https://devblogs.microsoft.com/oldnewthing/20181206-00/?p=100415 However it does not mention that `PAGE_NOACCESS` and `PAGE_READONLY` still result in an error on Windows, which is properly implemented in this MR. Only `WriteProcessMemory` offers this "service", `NtWriteVirtualMemory` will fail on non-writeable and executable regions (and already does so, except for the the mach server backend, which needs https://gitlab.winehq.org/wine/wine/-/merge_requests/4826 to also behave correctly here). -- v4: kernelbase: Flush instruction cache in WriteProcessMemory. kernelbase: Allow WriteProcessMemory to succeed on PAGE_EXECUTE and PAGE_EXECUTE_READ. https://gitlab.winehq.org/wine/wine/-/merge_requests/5222
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernel32/tests/virtual.c | 13 +++++++++++++ dlls/kernelbase/memory.c | 29 ++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/virtual.c b/dlls/kernel32/tests/virtual.c index a023104ad88..3c362b60ae3 100644 --- a/dlls/kernel32/tests/virtual.c +++ b/dlls/kernel32/tests/virtual.c @@ -83,6 +83,8 @@ static void test_VirtualAllocEx(void) DWORD old_prot; MEMORY_BASIC_INFORMATION info; HANDLE hProcess; + NTSTATUS status; + NTSTATUS (NTAPI *pNtWriteVirtualMemory)(HANDLE, void *, const void *, SIZE_T, SIZE_T *); /* Same process */ addr1 = VirtualAllocEx(GetCurrentProcess(), NULL, alloc_size, MEM_COMMIT, PAGE_EXECUTE_READWRITE); @@ -122,6 +124,17 @@ static void test_VirtualAllocEx(void) b = ReadProcessMemory(hProcess, addr1, src, 0, &bytes_read); ok(b && !bytes_read, "read failed: %lu\n", GetLastError()); + /* test write to read-only execute region */ + addr2 = VirtualAllocEx(hProcess, NULL, alloc_size, MEM_COMMIT, + PAGE_EXECUTE_READ); + ok(addr2 != NULL, "VirtualAllocEx error %lu\n", GetLastError()); + b = WriteProcessMemory(hProcess, addr2, src, alloc_size, &bytes_written); + ok(b, "WriteProcessMemory should succeed on a PAGE_EXECUTE_READ region. Failed error: %lu\n", GetLastError()); + pNtWriteVirtualMemory = (NTSTATUS (NTAPI *)(HANDLE, void *, const void *, SIZE_T, SIZE_T *)) + GetProcAddress(GetModuleHandleW(L"ntdll"), "NtWriteVirtualMemory"); + status = pNtWriteVirtualMemory(hProcess, addr2, src, alloc_size, &bytes_written); + ok(!NT_SUCCESS(status), "Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: %lu\n", status); + /* test invalid source buffers */ b = VirtualProtect( src + 0x2000, 0x2000, PAGE_NOACCESS, &old_prot ); diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c index 4f4bba9a13b..656fd5144b6 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -537,7 +537,34 @@ BOOL WINAPI DECLSPEC_HOTPATCH VirtualUnlock( void *addr, SIZE_T size ) BOOL WINAPI DECLSPEC_HOTPATCH WriteProcessMemory( HANDLE process, void *addr, const void *buffer, SIZE_T size, SIZE_T *bytes_written ) { - return set_ntstatus( NtWriteVirtualMemory( process, addr, buffer, size, bytes_written )); + NTSTATUS status, protect_status; + SIZE_T region_size = size; + ULONG old_prot; + void *base_addr = addr; + + status = NtWriteVirtualMemory( process, addr, buffer, size, bytes_written ); + + if (NT_SUCCESS(status)) + { + return set_ntstatus( status ); + } + + /* In the non-writeable code region case, Windows changes the protection to facilitate the write + * and then changes it back. */ + + protect_status = NtProtectVirtualMemory( process, &base_addr, ®ion_size, PAGE_EXECUTE_READWRITE, &old_prot ); + if (!NT_SUCCESS(protect_status)) return set_ntstatus( protect_status ); + + if ((old_prot & PAGE_EXECUTE) == PAGE_EXECUTE || + (old_prot & PAGE_EXECUTE_READ) == PAGE_EXECUTE_READ) + { + status = NtWriteVirtualMemory( process, addr, buffer, size, bytes_written ); + } + + protect_status = NtProtectVirtualMemory( process, &base_addr, ®ion_size, old_prot, &old_prot ); + if (!NT_SUCCESS(protect_status)) return set_ntstatus( protect_status ); + + return set_ntstatus( status ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5222
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernelbase/memory.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c index 656fd5144b6..6814d49fa2a 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -546,6 +546,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH WriteProcessMemory( HANDLE process, void *addr, co if (NT_SUCCESS(status)) { + NtFlushInstructionCache( process, addr, size ); return set_ntstatus( status ); } @@ -564,6 +565,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH WriteProcessMemory( HANDLE process, void *addr, co protect_status = NtProtectVirtualMemory( process, &base_addr, ®ion_size, old_prot, &old_prot ); if (!NT_SUCCESS(protect_status)) return set_ntstatus( protect_status ); + NtFlushInstructionCache( process, addr, size ); return set_ntstatus( status ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5222
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=144230 Your paranoid android. === debian11 (32 bit report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit ar:MA report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit de report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit fr report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit he:IL report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit hi:IN report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit ja:JP report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11 (32 bit zh:CN report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11b (32 bit WoW report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0 === debian11b (64 bit WoW report) === kernel32: virtual.c:136: Test failed: Expected NtWriteVirtualMemory to fail on PAGE_EXECUTE_READ, but got status: 0
Rebased and made it so that there are no performance regressions on x86_64 anymore. Did a bit more testing on arm64 Windows as well and am now relatively certain it does a Query + Write + Flush in the good case and a Query + Protect + Write + Protect + Flush in the bad case, just by adding up the timings and the fact that it does seem to always flush, which isn't really testable on x86_64. I don't think any application relies on this sequence of events though (hopefully), so it is probably good to leave it as is, given that it offers the same behaviour now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222#note_65479
This merge request was closed by Marc-Aurel Zent. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222
This is already implemented by 6ea77ec0864eb7870ce3418bf828077c2a072413 in the same way as I believe windows does it, so will be closing this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222#note_73714
participants (3)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠mzent)
-
Marvin