[PATCH v3 0/4] MR6767: WriteProcessMemory() without PROCESS_QUERY_INFORMATION access right.
This is an alternate solution to MR 6761 [1] (actually is based on comment in that MR). WriteProcessMemory() should succeed when process handle doesn't have PROCESS_QUERY_INFORMATION access right set (test attached). Plus some additional cleanup in test for ReadProcessMemory(). Feedback from Proton says that this patch solves the reported game issue. [1] https://gitlab.winehq.org/wine/wine/-/merge_requests/6761 -- v3: server: Amend process rights mapping. advapi32: Test some other cases of process access rights mapping. advapi32/tests: Fix typo in manifest constant. kernel32/tests: Don't hardcode page size in buffer size. https://gitlab.winehq.org/wine/wine/-/merge_requests/6767
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/virtual.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/virtual.c b/dlls/kernel32/tests/virtual.c index 12841133c05..cfbfcac0950 100644 --- a/dlls/kernel32/tests/virtual.c +++ b/dlls/kernel32/tests/virtual.c @@ -4368,13 +4368,15 @@ static void test_PrefetchVirtualMemory(void) static void test_ReadProcessMemory(void) { - BYTE buf[0x2000]; + BYTE *buf; DWORD old_prot; SIZE_T copied; HANDLE hproc; void *ptr; BOOL ret; + buf = malloc(2 * si.dwPageSize); + ok(buf != NULL, "OOM\n"); ret = DuplicateHandle(GetCurrentProcess(), GetCurrentProcess(), GetCurrentProcess(), &hproc, 0, FALSE, DUPLICATE_SAME_ACCESS); ok(ret, "DuplicateHandle failed %lu\n", GetLastError()); @@ -4403,6 +4405,7 @@ static void test_ReadProcessMemory(void) ret = VirtualFree(ptr, 0, MEM_RELEASE); ok(ret, "VirtualFree failed %lu\n", GetLastError()); CloseHandle(hproc); + free(buf); } START_TEST(virtual) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6767
From: Eric Pouech <epouech(a)codeweavers.com> (harmless, the actual values are the same). Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/advapi32/tests/security.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 0917b144648..90c553902cd 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -3118,7 +3118,7 @@ static void test_process_security_child(void) /* Test thread security */ handle = OpenThread( THREAD_TERMINATE, FALSE, GetCurrentThreadId() ); ok(handle != NULL, "OpenThread(THREAD_TERMINATE) with err:%ld\n", GetLastError()); - TEST_GRANTED_ACCESS( handle, PROCESS_TERMINATE ); + TEST_GRANTED_ACCESS( handle, THREAD_TERMINATE ); CloseHandle( handle ); handle = OpenThread( THREAD_SET_THREAD_TOKEN, FALSE, GetCurrentThreadId() ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6767
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/advapi32/tests/security.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 90c553902cd..04e9906450e 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -6154,6 +6154,16 @@ static void test_process_access(void) "expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got %#lx\n", access); CloseHandle(dup); + SetLastError( 0xdeadbeef ); + ret = DuplicateHandle(GetCurrentProcess(), process, GetCurrentProcess(), &dup, + PROCESS_VM_OPERATION | PROCESS_VM_WRITE, FALSE, 0); + ok(ret, "DuplicateHandle error %ld\n", GetLastError()); + access = get_obj_access(dup); + todo_wine + ok(access == (PROCESS_VM_OPERATION | PROCESS_VM_WRITE | PROCESS_QUERY_LIMITED_INFORMATION), + "expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got %#lx\n", access); + CloseHandle(dup); + TerminateProcess(process, 0); CloseHandle(process); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6767
From: Eric Pouech <epouech(a)codeweavers.com> PROCESS_VM_OPERATION | PROCESS_VM_WRITE grants PROCESS_QUERY_LIMITED_INFORMATION rights. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/advapi32/tests/security.c | 1 - server/process.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 04e9906450e..b0f964f7e55 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -6159,7 +6159,6 @@ static void test_process_access(void) PROCESS_VM_OPERATION | PROCESS_VM_WRITE, FALSE, 0); ok(ret, "DuplicateHandle error %ld\n", GetLastError()); access = get_obj_access(dup); - todo_wine ok(access == (PROCESS_VM_OPERATION | PROCESS_VM_WRITE | PROCESS_QUERY_LIMITED_INFORMATION), "expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got %#lx\n", access); CloseHandle(dup); diff --git a/server/process.c b/server/process.c index 49f5c75005f..f386239919d 100644 --- a/server/process.c +++ b/server/process.c @@ -805,6 +805,8 @@ static unsigned int process_map_access( struct object *obj, unsigned int access access = default_map_access( obj, access ); if (access & PROCESS_QUERY_INFORMATION) access |= PROCESS_QUERY_LIMITED_INFORMATION; if (access & PROCESS_SET_INFORMATION) access |= PROCESS_SET_LIMITED_INFORMATION; + if ((access & (PROCESS_VM_OPERATION | PROCESS_VM_WRITE)) == (PROCESS_VM_OPERATION | PROCESS_VM_WRITE)) + access |= PROCESS_QUERY_LIMITED_INFORMATION; return access; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6767
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149480 Your paranoid android. === w7u_2qxl (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w7u_adm (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w7u_el (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w8 (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w8adm (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w864 (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w1064v1507 (32 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w7pro64 (64 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w864 (64 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28 === w1064v1507 (64 bit report) === advapi32: security.c:6162: Test failed: expected PROCESS_QUERY_INFORMATION|PROCESS_QUERY_LIMITED_INFORMATION, got 0x28
On Tue Nov 5 10:09:10 2024 +0000, Dmitry Timoshkov wrote: > It should be possible to add a test for this. See > dlls/advapi32/tests/security.c,test_process_access() for an example. V3 pushed: - removed test in ntdll (we already have tests in kernel32 for QUERY_LIMITED_INFORMATION only access and VirtualQuery) - replaced with direct granted access rights test in advapi32 - added a fix for a typo I came across perusing the advapi32/test/security.c file -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6767#note_86614
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin