[PATCH v8 0/6] MR4826: Mach vm operations improvements
For all mach vm operations this removes the task suspend and resume, which are not needed. This uses `mach_vm_read_overwrite` to read into a caller-specified buffer, saving the `mach_vm_deallocate` call (bringing all read operations down to 1 syscall from 4). The only alignment restriction on `mach_vm_write` according to the original CMU documentation is that data is
[pointer to page aligned in array of bytes] An array of data to be written.
(In practice it also works with arbitrary addresses on macOS, but it probably doesn't hurt to follow the original specifications here). The only other reference that these read/writes should be page-aligned is from the GNU Hurd documentation
The current implementation requires that address, data and data_count all be page-aligned. Otherwise, KERN_INVALID_ARGUMENT is returned.
which I assume was the reason why this was originally done (plus it sounds to me like they will fix that in the future and 4fe19777 already broke GNU Hurd support anyways, if that was supposed to be working). Also this includes the missing mach part of 5b1f3b14, which was only applied to the ptrace backend, and together with the `write_process_memory` rework, this gets rid of all fixmes in mach.c -- v8: server: Do not page align address in write_process_memory. server: Do not suspend mach task in write_process_memory. server: Use mach_vm_read_overwrite in get_selector_entry. server: Do not suspend mach task in get_selector_entry. server: Use mach_vm_read_overwrite in read_process_memory. server: Do not suspend mach task in read_process_memory. https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/server/mach.c b/server/mach.c index e4bb0a46d40..44a9a589952 100644 --- a/server/mach.c +++ b/server/mach.c @@ -411,12 +411,6 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t return 0; } - if ((ret = task_suspend( process_port )) != KERN_SUCCESS) - { - mach_set_error( ret ); - return 0; - } - offset = ptr % page_size; aligned_address = (mach_vm_address_t)(ptr - offset); aligned_size = (size + offset + page_size - 1) / page_size * page_size; @@ -428,7 +422,6 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t memcpy( dest, (char *)data + offset, size ); mach_vm_deallocate( mach_task_self(), data, bytes_read ); } - task_resume( process_port ); return (ret == KERN_SUCCESS); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/server/mach.c b/server/mach.c index 44a9a589952..747754035d8 100644 --- a/server/mach.c +++ b/server/mach.c @@ -392,12 +392,7 @@ int send_thread_signal( struct thread *thread, int sig ) int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, char *dest ) { kern_return_t ret; - mach_msg_type_number_t bytes_read; - mach_vm_offset_t offset; - vm_offset_t data; - mach_vm_address_t aligned_address; - mach_vm_size_t aligned_size; - unsigned int page_size = get_page_size(); + mach_vm_size_t bytes_read; mach_port_t process_port = get_process_port( process ); if (!process_port) @@ -411,20 +406,12 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t return 0; } - offset = ptr % page_size; - aligned_address = (mach_vm_address_t)(ptr - offset); - aligned_size = (size + offset + page_size - 1) / page_size * page_size; - - ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); - if (ret != KERN_SUCCESS) mach_set_error( ret ); - else - { - memcpy( dest, (char *)data + offset, size ); - mach_vm_deallocate( mach_task_self(), data, bytes_read ); - } + ret = mach_vm_read_overwrite( process_port, (mach_vm_address_t)ptr, (mach_vm_size_t)size, (mach_vm_address_t)dest, &bytes_read ); + mach_set_error( ret ); return (ret == KERN_SUCCESS); } + /* write data to a process memory space */ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src ) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/server/mach.c b/server/mach.c index 747754035d8..cf3f647e658 100644 --- a/server/mach.c +++ b/server/mach.c @@ -523,25 +523,20 @@ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, return; } - if ((ret = task_suspend( process_port )) == KERN_SUCCESS) - { - mach_vm_offset_t offset = process->ldt_copy % page_size; - mach_vm_address_t aligned_address = (mach_vm_address_t)(process->ldt_copy - offset); - mach_vm_size_t aligned_size = (total_size + offset + page_size - 1) / page_size * page_size; + mach_vm_offset_t offset = process->ldt_copy % page_size; + mach_vm_address_t aligned_address = (mach_vm_address_t)(process->ldt_copy - offset); + mach_vm_size_t aligned_size = (total_size + offset + page_size - 1) / page_size * page_size; - ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); - if (ret != KERN_SUCCESS) mach_set_error( ret ); - else - { - const int *ldt = (const int *)((char *)data + offset); - memcpy( base, ldt + entry, sizeof(int) ); - memcpy( limit, ldt + entry + 8192, sizeof(int) ); - memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 ); - mach_vm_deallocate( mach_task_self(), data, bytes_read ); - } - task_resume( process_port ); + ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); + if (ret != KERN_SUCCESS) mach_set_error( ret ); + else + { + const int *ldt = (const int *)((char *)data + offset); + memcpy( base, ldt + entry, sizeof(int) ); + memcpy( limit, ldt + entry + 8192, sizeof(int) ); + memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 ); + mach_vm_deallocate( mach_task_self(), data, bytes_read ); } - else mach_set_error( ret ); } #endif /* USE_MACH */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/server/mach.c b/server/mach.c index cf3f647e658..6a576b9426c 100644 --- a/server/mach.c +++ b/server/mach.c @@ -506,10 +506,9 @@ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, { const unsigned int total_size = (2 * sizeof(int) + 1) * 8192; struct process *process = thread->process; - unsigned int page_size = get_page_size(); - vm_offset_t data; + mach_vm_address_t data; kern_return_t ret; - mach_msg_type_number_t bytes_read; + mach_vm_size_t bytes_read; mach_port_t process_port = get_process_port( thread->process ); if (!process->ldt_copy || !process_port) @@ -519,24 +518,27 @@ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, } if (entry >= 8192) { - set_error( STATUS_INVALID_PARAMETER ); /* FIXME */ + set_error( STATUS_ACCESS_VIOLATION ); return; } - mach_vm_offset_t offset = process->ldt_copy % page_size; - mach_vm_address_t aligned_address = (mach_vm_address_t)(process->ldt_copy - offset); - mach_vm_size_t aligned_size = (total_size + offset + page_size - 1) / page_size * page_size; + if (!(data = (mach_vm_address_t)malloc( total_size ))) + { + set_error( STATUS_NO_MEMORY ); + return; + } - ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); + ret = mach_vm_read_overwrite( process_port, (mach_vm_address_t)process->ldt_copy, (mach_vm_size_t)total_size, data, &bytes_read ); if (ret != KERN_SUCCESS) mach_set_error( ret ); else { - const int *ldt = (const int *)((char *)data + offset); + const int *ldt = (const int *)data; memcpy( base, ldt + entry, sizeof(int) ); memcpy( limit, ldt + entry + 8192, sizeof(int) ); memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 ); - mach_vm_deallocate( mach_task_self(), data, bytes_read ); } + + free( (void *)data ); } #endif /* USE_MACH */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/server/mach.c b/server/mach.c index 6a576b9426c..be576f867f8 100644 --- a/server/mach.c +++ b/server/mach.c @@ -441,12 +441,6 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t aligned_address = (mach_vm_address_t)(ptr - offset); aligned_size = (size + offset + page_size - 1) / page_size * page_size; - if ((ret = task_suspend( process_port )) != KERN_SUCCESS) - { - mach_set_error( ret ); - return 0; - } - ret = mach_vm_read( process_port, aligned_address, aligned_size, &task_mem, &bytes_read ); if (ret != KERN_SUCCESS) { @@ -490,13 +484,11 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); /* restore protection */ mach_vm_protect( process_port, aligned_address, aligned_size, 0, info.protection ); - task_resume( process_port ); return 1; } failed: if (task_mem) mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); - task_resume( process_port ); return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 66 +++++++-------------------------------------------- 1 file changed, 9 insertions(+), 57 deletions(-) diff --git a/server/mach.c b/server/mach.c index be576f867f8..b06ea4cfdb6 100644 --- a/server/mach.c +++ b/server/mach.c @@ -416,15 +416,9 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src ) { kern_return_t ret; - mach_vm_address_t aligned_address, region_address; - mach_vm_size_t aligned_size, region_size; - mach_msg_type_number_t info_size, bytes_read; - mach_vm_offset_t offset; - vm_offset_t task_mem = 0; - struct vm_region_basic_info_64 info; - mach_port_t dummy; unsigned int page_size = get_page_size(); mach_port_t process_port = get_process_port( process ); + mach_vm_offset_t data; if (!process_port) { @@ -436,60 +430,18 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t set_error( STATUS_ACCESS_DENIED ); return 0; } - - offset = ptr % page_size; - aligned_address = (mach_vm_address_t)(ptr - offset); - aligned_size = (size + offset + page_size - 1) / page_size * page_size; - - ret = mach_vm_read( process_port, aligned_address, aligned_size, &task_mem, &bytes_read ); - if (ret != KERN_SUCCESS) - { - mach_set_error( ret ); - goto failed; - } - region_address = aligned_address; - info_size = sizeof(info); - ret = mach_vm_region( process_port, ®ion_address, ®ion_size, VM_REGION_BASIC_INFO_64, - (vm_region_info_t)&info, &info_size, &dummy ); - if (ret != KERN_SUCCESS) + if (posix_memalign( (void **)&data, page_size, size )) { - mach_set_error( ret ); - goto failed; - } - if (region_address > aligned_address || - region_address + region_size < aligned_address + aligned_size) - { - /* FIXME: should support multiple regions */ - set_error( ERROR_ACCESS_DENIED ); - goto failed; - } - ret = mach_vm_protect( process_port, aligned_address, aligned_size, 0, VM_PROT_READ | VM_PROT_WRITE ); - if (ret != KERN_SUCCESS) - { - mach_set_error( ret ); - goto failed; + set_error( STATUS_NO_MEMORY ); + return 0; } - /* FIXME: there's an optimization that can be made: check first and last */ - /* pages for writability; read first and last pages; write interior */ - /* pages to task without ever reading&modifying them; if that succeeds, */ - /* modify first and last pages and write them. */ - - memcpy( (char*)task_mem + offset, src, size ); + memcpy( (void *)data, src, size ); - ret = mach_vm_write( process_port, aligned_address, task_mem, bytes_read ); - if (ret != KERN_SUCCESS) mach_set_error( ret ); - else - { - mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); - /* restore protection */ - mach_vm_protect( process_port, aligned_address, aligned_size, 0, info.protection ); - return 1; - } - -failed: - if (task_mem) mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); - return 0; + ret = mach_vm_write( process_port, (mach_vm_address_t)ptr, data, (mach_msg_type_number_t)size ); + free( (void *)data ); + mach_set_error( ret ); + return (ret == KERN_SUCCESS); } /* retrieve an LDT selector entry */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
On Mon Dec 2 16:08:48 2024 +0000, Brendan Shanks wrote:
Can you put the removal of `task_suspend()` and `task_resume()` (I guess for all 3 functions) into a separate commit? Thanks for looking over it, I tried splitting them up where it made sense.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826#note_89443
On Mon Dec 2 16:17:04 2024 +0000, Brendan Shanks wrote:
I'd still like to test these out (particularly with some games/launchers that are known to use cross-process memory writes) FWIW I originally looked into this since `NtWriteVirtualMemory` was misbehaving sometimes for me with the current implementation.
Also [XNU cross-process vm memory write tests](https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b...) do not seem to suspend/resume the target task before vm read/writes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826#note_89446
participants (2)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠mzent)