[PATCH v2 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). -- v2: kernelbase: Flush instruction cache when necessary in WriteProcessMemory. kernelbase: Allow WriteProcessMemory to succeed on 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 | 7 +++++++ dlls/kernelbase/memory.c | 38 ++++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/virtual.c b/dlls/kernel32/tests/virtual.c index a023104ad88..7a23962be54 100644 --- a/dlls/kernel32/tests/virtual.c +++ b/dlls/kernel32/tests/virtual.c @@ -122,6 +122,13 @@ 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()); + /* 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..d5cf421faae 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -537,7 +537,43 @@ 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; + MEMORY_BASIC_INFORMATION mbi; + SIZE_T res_len, region_size = size; + ULONG old_prot; + void *base_addr = addr; + + status = NtQueryVirtualMemory( process, addr, MemoryBasicInformation, &mbi, sizeof(mbi), &res_len ); + + if (!NT_SUCCESS(status)) return set_ntstatus( status ); + + if ((mbi.Protect & PAGE_NOACCESS) == PAGE_NOACCESS || + (mbi.Protect & PAGE_READONLY) == PAGE_READONLY) + return set_ntstatus( STATUS_ACCESS_VIOLATION ); + + if ((mbi.Protect & PAGE_READWRITE) == PAGE_READWRITE || + (mbi.Protect & PAGE_WRITECOPY) == PAGE_WRITECOPY) + return set_ntstatus( NtWriteVirtualMemory( process, addr, buffer, size, bytes_written ) ); + + if ((mbi.Protect & PAGE_EXECUTE_READWRITE) == PAGE_EXECUTE_READWRITE || + (mbi.Protect & PAGE_EXECUTE_WRITECOPY) == PAGE_EXECUTE_WRITECOPY) + { + status = NtWriteVirtualMemory( process, addr, buffer, size, bytes_written ); + 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_READWRITE, &old_prot ); + if (!NT_SUCCESS(protect_status)) return set_ntstatus( protect_status ); + + 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 d5cf421faae..ccc8799405d 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -559,6 +559,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH WriteProcessMemory( HANDLE process, void *addr, co (mbi.Protect & PAGE_EXECUTE_WRITECOPY) == PAGE_EXECUTE_WRITECOPY) { status = NtWriteVirtualMemory( process, addr, buffer, size, bytes_written ); + NtFlushInstructionCache( process, addr, size ); return set_ntstatus( status ); } @@ -573,6 +574,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
Ah the difference with and without this MR is exactly the difference between NtWriteVirtualMemory and WriteProcessMemory above (within margin of error). Interestingly with arm64 Windows, allocating some virtual memory as PAGE_EXECUTE_READWRITE and then writing instructions into to return 1, shows that Windows most likely indeed does flush the instruction cache; I get the following behaviour consistently (10/10 times): * memcpy: illegal instruction exception * memcpy + NtFlushInstructionCache: returns 1. * NtWriteVirtualMemory: returns 0. * NtWriteVirtualMemory + NtFlushInstructionCache: returns 1. * WriteProcessMemory: returns 1. * WriteProcessMemory + NtFlushInstructionCache: returns 1. Commit should be split appropriately now, will add a few more tests later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222#note_63372
Is it preferred that all of the tests are in their own commit, or should everything be part of 8a019ee32a44864fa8934161fda762aeba1ac1b6 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222#note_63373
I wonder if we shouldn't do it in the fallback case of a failing write
Querying first has the nice side-benefit of fixing the mach backend currently succeeding in the PAGE_READONLY and PAGE_NOACCESS cases. I am also not too sure, but I think the procfs backend might (partially) write out memory even if it fails... I think it is cleaner to not attempt it at all, if it is clear it would fail. Otherwise querying first should be the lowest number of "syscalls" for well-behaved applications (2 + sometimes flush) and then (4 + flush) for the bad case. To unprotect first unconditionally would be always (3 + sometimes flush). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222#note_63380
You mentioned that this trick is done on kernelbase and not in NtWriteVirtualMemory level. I think we don't have pre-existing NtWriteVirtualMemory tests, maybe worth adding one right at the same place where you test WriteProcessMemory to make it clear? I guess pre-querying is better than unconditionally changing protection. First, "well-behaved" case performance already mentioned which I suppose is the majority of practical usage at least now, since things mostly work without this functionality. Then, you are changing the protection PAGE_READWRITE when the page can't be written with existing permissions. Are you sure is it correct and, e. g., it shouldn't be PAGE_EXECUTEREADWRITE when appropriate? I guess it can be tested with a thread repeatedly calling a PAGE_EXECUTEREAD code region being written with the same data instructions. The behaviour like in the present patch should make that cause access violation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5222#note_63388
participants (3)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠mzent)
-
Paul Gofman (＠gofman)