[PATCH v3 0/2] MR8553: 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) -- v3: kernelbase/tests: Add tests for 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 | 12 ++++++++++++ include/winbase.h | 1 + 3 files changed, 14 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..ef3a644907e 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -572,6 +572,18 @@ 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 ) +{ + if (new_prot & ( PAGE_EXECUTE_READWRITE | PAGE_EXECUTE_WRITECOPY ) || !old_prot) + { + return set_ntstatus( STATUS_INVALID_PARAMETER ); + } + 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 | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/dlls/kernelbase/tests/process.c b/dlls/kernelbase/tests/process.c index 90d6b5204eb..1be3ad276b1 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,35 @@ static void test_VirtualAlloc2FromApp(void) } } +static void test_VirtualProtectFromApp(void) +{ + ULONG old_prot; + void *p; + BOOL ret; + + if (!pVirtualAllocFromApp || !pVirtualProtectFromApp) + { + win_skip("VirtualProtectFromApp is not available.\n"); + return; + } + + SetLastError(0xdeadbeef); + p = pVirtualAllocFromApp(NULL, 0x1000, MEM_RESERVE, 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, 0x1000, PAGE_EXECUTE_READWRITE, &old_prot); + ok(!ret, "Unexpected protection PAGE_EXECUTE_READWRITE\n"); + + ret = pVirtualProtectFromApp(p, 0x1000, PAGE_EXECUTE_WRITECOPY, &old_prot); + ok(!ret, "Unexpected protection PAGE_EXECUTE_WRITECOPY\n"); + + 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 +631,7 @@ static void init_funcs(void) X(VirtualAlloc2); X(VirtualAlloc2FromApp); X(VirtualAllocFromApp); + X(VirtualProtectFromApp); X(UnmapViewOfFile2); hmod = GetModuleHandleA("ntdll.dll"); @@ -618,6 +649,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
not a full review, just some comments to enhance the tests: - the tests could check if the new protections are effective (IsBadReadPtr/IsBadWritePtr could be used here) - 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) - 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. - 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) you should also update the merge request (and not close it and reopen a new one), so that we don't lose tracks of comments between updates TIA -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8553#note_109751
participants (3)
-
eric pouech (＠epo) -
Kareem Aladli
-
Kareem Aladli (＠karim_adli)