[PATCH v4 0/3] MR8553: Draft: kernelbase: implement VirtualProtectFromApp
This implements the VirtualProtectFromApp function in kernelbase, which was previously a commented out stub. Even though this function is meant for UWP apps, it is usable in desktop apps, as specified in [Windows documentation](https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-v...). Fixes Wine-Bug: [https://bugs.winehq.org/show_bug.cgi?id=58482](https://bugs.winehq.org/show_bug.cgi?id=58482) -- v4: ntdll: Set old_prot in NtProtectVirtualMemory() to PAGE_NOACCESS if it fails kernelbase/tests: Add tests for VirtualProtectFromApp kernelbase: implement VirtualProtectFromApp https://gitlab.winehq.org/wine/wine/-/merge_requests/8553
From: Kareem Aladli <karimri(a)protonmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58482 Signed-off-by: Kareem Aladli <karimri(a)protonmail.com> --- dlls/kernelbase/kernelbase.spec | 2 +- dlls/kernelbase/memory.c | 9 +++++++++ include/winbase.h | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/dlls/kernelbase/kernelbase.spec b/dlls/kernelbase/kernelbase.spec index 545c30a1eb8..48471834735 100644 --- a/dlls/kernelbase/kernelbase.spec +++ b/dlls/kernelbase/kernelbase.spec @@ -1729,7 +1729,7 @@ @ stdcall VirtualLock(ptr long) @ stdcall VirtualProtect(ptr long long ptr) @ stdcall VirtualProtectEx(long ptr long long ptr) -# @ stub VirtualProtectFromApp +@ stdcall VirtualProtectFromApp(ptr long long ptr) @ stdcall VirtualQuery(ptr ptr long) @ stdcall VirtualQueryEx(long ptr ptr long) @ stdcall VirtualUnlock(ptr long) diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c index 9bf8e291cbe..34c83eb428b 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -572,6 +572,15 @@ BOOL WINAPI DECLSPEC_HOTPATCH VirtualProtectEx( HANDLE process, void *addr, SIZE return set_ntstatus( NtProtectVirtualMemory( process, &addr, &size, new_prot, old_prot )); } +/*********************************************************************** + * VirtualProtectFromApp (kernelbase.@) + */ +BOOL WINAPI DECLSPEC_HOTPATCH VirtualProtectFromApp( void *addr, SIZE_T size, + ULONG new_prot, ULONG *old_prot ) +{ + /* Contrary to the documentation, VirtualProtectFromApp allows write+execute on desktop. */ + return VirtualProtect( addr, size, new_prot, old_prot ); +} /*********************************************************************** * VirtualQuery (kernelbase.@) diff --git a/include/winbase.h b/include/winbase.h index 29a3ea8fbce..b4fae70dda1 100644 --- a/include/winbase.h +++ b/include/winbase.h @@ -2479,6 +2479,7 @@ WINBASEAPI BOOL WINAPI VirtualFreeEx(HANDLE,LPVOID,SIZE_T,DWORD); WINBASEAPI BOOL WINAPI VirtualLock(LPVOID,SIZE_T); WINBASEAPI BOOL WINAPI VirtualProtect(LPVOID,SIZE_T,DWORD,LPDWORD); WINBASEAPI BOOL WINAPI VirtualProtectEx(HANDLE,LPVOID,SIZE_T,DWORD,LPDWORD); +WINBASEAPI BOOL WINAPI VirtualProtectFromApp(LPVOID,SIZE_T,ULONG,PULONG); WINBASEAPI SIZE_T WINAPI VirtualQuery(LPCVOID,PMEMORY_BASIC_INFORMATION,SIZE_T); WINBASEAPI SIZE_T WINAPI VirtualQueryEx(HANDLE,LPCVOID,PMEMORY_BASIC_INFORMATION,SIZE_T); WINBASEAPI BOOL WINAPI VirtualUnlock(LPVOID,SIZE_T); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8553
From: Kareem Aladli <karimri(a)protonmail.com> --- dlls/kernelbase/tests/process.c | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/dlls/kernelbase/tests/process.c b/dlls/kernelbase/tests/process.c index 90d6b5204eb..e4a5cf0d557 100644 --- a/dlls/kernelbase/tests/process.c +++ b/dlls/kernelbase/tests/process.c @@ -38,6 +38,7 @@ static LPVOID (WINAPI *pMapViewOfFile3)(HANDLE, HANDLE, PVOID, ULONG64 offset, S static LPVOID (WINAPI *pVirtualAlloc2)(HANDLE, void *, SIZE_T, DWORD, DWORD, MEM_EXTENDED_PARAMETER *, ULONG); static LPVOID (WINAPI *pVirtualAlloc2FromApp)(HANDLE, void *, SIZE_T, DWORD, DWORD, MEM_EXTENDED_PARAMETER *, ULONG); static PVOID (WINAPI *pVirtualAllocFromApp)(PVOID, SIZE_T, DWORD, DWORD); +static BOOL (WINAPI *pVirtualProtectFromApp)(LPVOID,SIZE_T,ULONG,PULONG); static HANDLE (WINAPI *pOpenFileMappingFromApp)( ULONG, BOOL, LPCWSTR); static HANDLE (WINAPI *pCreateFileMappingFromApp)(HANDLE, PSECURITY_ATTRIBUTES, ULONG, ULONG64, PCWSTR); static LPVOID (WINAPI *pMapViewOfFileFromApp)(HANDLE, ULONG, ULONG64, SIZE_T); @@ -481,6 +482,43 @@ static void test_VirtualAlloc2FromApp(void) } } +static void test_VirtualProtectFromApp(void) +{ + ULONG old_prot; + void *p; + BOOL ret; + + if (!pVirtualProtectFromApp) + { + win_skip("VirtualProtectFromApp is not available.\n"); + return; + } + + SetLastError(0xdeadbeef); + p = VirtualAlloc(NULL, 0x1000, MEM_COMMIT, PAGE_READWRITE); + ok(p && GetLastError() == 0xdeadbeef, "Got unexpected mem %p, GetLastError() %lu.\n", p, GetLastError()); + ret = pVirtualProtectFromApp(p, 0x1000, PAGE_READONLY, &old_prot); + ok(ret && old_prot == PAGE_READWRITE, "Failed to change protection old_prot %lu, GetLastError() %lu\n", + old_prot, GetLastError()); + + ret = pVirtualProtectFromApp(p, 0x100000, PAGE_READWRITE, &old_prot); + ok(!ret && old_prot == PAGE_NOACCESS, "Call worked with overflowing size old_prot %lu\n", old_prot); + + ret = pVirtualProtectFromApp(p, 0x1000, PAGE_EXECUTE_READ, NULL); + ok(!ret, "Call worked without old_prot\n"); + + ret = pVirtualProtectFromApp(p, 0x1000, PAGE_GUARD, &old_prot); + ok(!ret && old_prot == PAGE_NOACCESS, "Call worked with an invalid new_prot parameter old_prot %lu\n", old_prot); + + // this works on desktop but not on uwp + ret = pVirtualProtectFromApp(p, 0x1000, PAGE_EXECUTE_READWRITE, &old_prot); + ok(ret && old_prot == PAGE_READONLY, "Failed to change protection old_prot %lu, GetLastError() %lu\n", + old_prot, GetLastError()); + + ret = VirtualFree(p, 0, MEM_RELEASE); + ok(ret, "Failed to free mem error %lu.\n", GetLastError()); +} + static void test_OpenFileMappingFromApp(void) { OBJECT_BASIC_INFORMATION info; @@ -601,6 +639,7 @@ static void init_funcs(void) X(VirtualAlloc2); X(VirtualAlloc2FromApp); X(VirtualAllocFromApp); + X(VirtualProtectFromApp); X(UnmapViewOfFile2); hmod = GetModuleHandleA("ntdll.dll"); @@ -618,6 +657,7 @@ START_TEST(process) test_VirtualAlloc2(); test_VirtualAllocFromApp(); test_VirtualAlloc2FromApp(); + test_VirtualProtectFromApp(); test_OpenFileMappingFromApp(); test_CreateFileMappingFromApp(); test_MapViewOfFileFromApp(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8553
From: Kareem Aladli <karimri(a)protonmail.com> --- dlls/ntdll/unix/virtual.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index e733e3ffdd6..c5408cbb5da 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -5350,6 +5350,7 @@ NTSTATUS WINAPI NtProtectVirtualMemory( HANDLE process, PVOID *addr_ptr, SIZE_T if (!old_prot) return STATUS_ACCESS_VIOLATION; + *old_prot = PAGE_NOACCESS; if (process != NtCurrentProcess()) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8553
the tests could check if the new protections are effective (IsBadReadPtr/IsBadWritePtr could be used here)
IsBadReadPtr seems to be obsolete, and checking old_prot in the next test should be enough to check protections are in place.
I don't see a test to check behavior when old_prot is NULL (some APIs crashes, some APIs check the pointer and return an error, some support the NULL pointer by not returning the old value) (if it's crashing on native, this can be marked with a comment in the test file)
It's the same as VirtualProtect/VirtualProtectEx, it checks the pointer and returns an error.
the implementation will fail when eg PAGE_GUARD is passed, which is not what MSDN states. But an additional test is always better than believing MSDN which is not always very accurate.
Sure, I added an additional test.
is old_prot modified in case of failure? (eg. if PAGE_EXECUTE_READWRITE is passed, it could be that old_prot is still written to with current protection, or maybe it's not written to)
It is, I was investigating this on a virtual machine. In any case of failure, old_prot is set to PAGE_NOACCESS. I changed NtProtectVirtualMemory() to match this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8553#note_109773
participants (2)
-
Kareem Aladli -
Kareem Aladli (＠karim_adli)