[PATCH 0/1] MR5222: kernelbase: Allow WriteProcessMemory to succeed on PAGE_EXECUTE_READ.

[Marc-Aurel Zent (＠mzent)]

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).

[PATCH 1/1] kernelbase: Allow WriteProcessMemory to succeed on PAGE_EXECUTE_READ.

[Marc-Aurel Zent]

From: Marc-Aurel Zent

---
 dlls/kernel32/tests/virtual.c |  7 ++++++
 dlls/kernelbase/memory.c      | 40 ++++++++++++++++++++++++++++++++++-
 2 files changed, 46 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..ccc8799405d 100644
--- a/dlls/kernelbase/memory.c
+++ b/dlls/kernelbase/memory.c
@@ -537,7 +537,45 @@ 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 );
+        NtFlushInstructionCache( process, addr, size );
+        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, &region_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, &region_size, old_prot, &old_prot );
+    if (!NT_SUCCESS(protect_status)) return set_ntstatus( protect_status );
+
+    NtFlushInstructionCache( process, addr, size );
+    return set_ntstatus( status );
 }

[eric pouech (＠epo)]

a couple of comments:

* IMO the bits for calling NtFlushInstructionCache should be put in a separate change
* do you have any indication that native does flush the insn cache? (can be tricky to test). and if so, is it done in kernelbase or ntdll?
* I'm concerned with the call to query the protection before the write op. did you made some perf measurement here? I wonder if we shouldn't do it in the fallback case of a failing write? or unprotect first unconditionnally, or...
* tests would be welcomed too
* as a test case, it would be interesting to have a write across two pages, each one with a different protection, so see how it should behave (your code assumes that all pages have the same protection as the first one). maybe it's how native behaves, but it's worth checking it

[Marc-Aurel Zent (＠mzent)]

Can put the addition of NtFlushInstructionCache into a separate commit, while doing a bit of research on the differences between NtWriteVirtualMemory and WriteProcessMemory IIRC there was mention somewhere that in addition to changing the protection, it also flushes the instruction cache (but can try to see if I can find some more evidence on arm64 Windows for that).

Have been thinking about unconditionally changing the protection to be writeable and then writing, however that would be three Nt* operations versus two with querying first, for well-behaved applications.
Here are some intra-process performance numbers in microseconds for a writable 1024 bytes region with this change (10000 iterations on x86_64):

|  | native | wine |
| ------ | ------ | ------ |
|WriteProcessMemory|1.414879|22.287070|
|NtWriteVirtualMemory|1.053687|19.914850|

Can add a few more tests as well, thanks for looking over it!

[eric pouech (＠epo)]

in the perf area, I was more thinking of wine with and without the MR...

but the cases where a debugger writes to memory are way less numerous than the read ones, so we likely can afford some extra time here