[PATCH v2 0/2] 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).

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

[PATCH v2 1/2] 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      | 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, &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 );
+
+    return set_ntstatus( status );
 }

[PATCH v2 2/2] kernelbase: Flush instruction cache when necessary in WriteProcessMemory.

[Marc-Aurel Zent]

From: Marc-Aurel Zent

---
 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, &region_size, old_prot, &old_prot );
     if (!NT_SUCCESS(protect_status)) return set_ntstatus( protect_status );

+    NtFlushInstructionCache( process, addr, size );
     return set_ntstatus( status );
 }

[Marc-Aurel Zent (＠mzent)]

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.

[Marc-Aurel Zent (＠mzent)]

Is it preferred that all of the tests are in their own commit, or should everything be part of 8a019ee32a44864fa8934161fda762aeba1ac1b6

[Marc-Aurel Zent (＠mzent)]

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

[Paul Gofman (＠gofman)]

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.