[PATCH v11 0/7] 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 -- v11: server: Work around macOS W^X limitations in write_process_memory. 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 | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/server/mach.c b/server/mach.c index 44a9a589952..796f82ac64a 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,17 +406,8 @@ 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); } -- 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 796f82ac64a..cbd36d6ecd8 100644 --- a/server/mach.c +++ b/server/mach.c @@ -522,25 +522,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 cbd36d6ecd8..16abc3f56fb 100644 --- a/server/mach.c +++ b/server/mach.c @@ -505,10 +505,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) @@ -518,24 +517,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 16abc3f56fb..1dca47cd5af 100644 --- a/server/mach.c +++ b/server/mach.c @@ -440,12 +440,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) { @@ -489,13 +483,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 1dca47cd5af..bd7b7e33037 100644 --- a/server/mach.c +++ b/server/mach.c @@ -415,15 +415,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) { @@ -435,60 +429,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
From: Marc-Aurel Zent <mzent(a)codeweavers.com> Also works around a Rosetta 2 bug, where code execution can remove VM_PROT_WRITE permissions from a page. --- server/mach.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/server/mach.c b/server/mach.c index bd7b7e33037..51206231330 100644 --- a/server/mach.c +++ b/server/mach.c @@ -438,6 +438,102 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t memcpy( (void *)data, src, size ); ret = mach_vm_write( process_port, (mach_vm_address_t)ptr, data, (mach_msg_type_number_t)size ); + + /* + * On arm64 macOS, enabling execute permission for a memory region automatically disables write + * permission for that region. This can also happen under Rosetta sometimes. + * In that case mach_vm_write returns KERN_INVALID_ADDRESS. + */ + + if (ret == KERN_INVALID_ADDRESS) + { + mach_vm_address_t current_address = (mach_vm_address_t)ptr; + mach_vm_address_t region_address = current_address; + mach_vm_size_t region_size, write_size; + vm_region_basic_info_data_t info; + mach_msg_type_number_t info_count = VM_REGION_BASIC_INFO_COUNT_64; + mach_port_t object_name; + data_size_t remaining_size = size; + + ret = mach_vm_region( process_port, ®ion_address, ®ion_size, VM_REGION_BASIC_INFO_64, + (vm_region_info_t)&info, &info_count, &object_name ); + if (ret != KERN_SUCCESS) + goto out; + + /* + * Actually check that everything is sane before suspending. + * KERN_INVALID_ADDRESS can also be returned when address is illegal or + * specifies a non-allocated region. + */ + if (region_address > current_address || + region_address + region_size <= current_address) + { + ret = KERN_INVALID_ADDRESS; + goto out; + } + + /* + * FIXME: Rosetta can turn RWX pages into R-X pages during execution. + * For now we will just have to ignore failures due to the wrong + * protection here. + */ + if (!is_rosetta() && !(info.protection & VM_PROT_WRITE)) + { + ret = KERN_PROTECTION_FAILURE; + goto out; + } + + /* The following operations should seem atomic from the perspective of the + * target process. */ + if ((ret = task_suspend( process_port )) != KERN_SUCCESS) + goto out; + + /* Iterate over all applicable memory regions until the write is completed. */ + while (remaining_size) + { + region_address = current_address; + info_count = VM_REGION_BASIC_INFO_COUNT_64; + ret = mach_vm_region( process_port, ®ion_address, ®ion_size, VM_REGION_BASIC_INFO_64, + (vm_region_info_t)&info, &info_count, &object_name ); + if (ret != KERN_SUCCESS) break; + + if (region_address > current_address || + region_address + region_size <= current_address) + { + ret = KERN_INVALID_ADDRESS; + break; + } + + /* FIXME: See the above Rosetta remark. */ + if (!is_rosetta() && !(info.protection & VM_PROT_WRITE)) + { + ret = KERN_PROTECTION_FAILURE; + break; + } + + write_size = region_size - (current_address - region_address); + if (write_size > remaining_size) write_size = remaining_size; + + ret = mach_vm_protect( process_port, current_address, write_size, 0, + VM_PROT_READ | VM_PROT_WRITE ); + if (ret != KERN_SUCCESS) break; + + ret = mach_vm_write( process_port, current_address, + data + (current_address - (mach_vm_address_t)ptr), write_size ); + if (ret != KERN_SUCCESS) break; + + ret = mach_vm_protect( process_port, current_address, write_size, 0, + info.protection ); + if (ret != KERN_SUCCESS) break; + + current_address += write_size; + remaining_size -= write_size; + } + + task_resume( process_port ); + } + +out: free( (void *)data ); mach_set_error( ret ); return (ret == KERN_SUCCESS); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
Brendan Shanks (@bshanks) commented about server/mach.c:
+ if (ret != KERN_SUCCESS) + goto out; + + /* + * Actually check that everything is sane before suspending. + * KERN_INVALID_ADDRESS can also be returned when address is illegal or + * specifies a non-allocated region. + */ + if (region_address > current_address || + region_address + region_size <= current_address) + { + ret = KERN_INVALID_ADDRESS; + goto out; + } + + /* extra space at the end of this line also
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826#note_89921
Brendan Shanks (@bshanks) commented about server/mach.c:
+ if (ret == KERN_INVALID_ADDRESS) + { + mach_vm_address_t current_address = (mach_vm_address_t)ptr; + mach_vm_address_t region_address = current_address; + mach_vm_size_t region_size, write_size; + vm_region_basic_info_data_t info; + mach_msg_type_number_t info_count = VM_REGION_BASIC_INFO_COUNT_64; + mach_port_t object_name; + data_size_t remaining_size = size; + + ret = mach_vm_region( process_port, ®ion_address, ®ion_size, VM_REGION_BASIC_INFO_64, + (vm_region_info_t)&info, &info_count, &object_name ); + if (ret != KERN_SUCCESS) + goto out; + + /* git warned for an extra space at the end of this line
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826#note_89920
This merge request was approved by Brendan Shanks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
On Thu Dec 5 22:00:11 2024 +0000, Marc-Aurel Zent wrote:
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. With the latest Rosetta workaround, this is able to activate Phantasmat successfully
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826#note_89922
Other than those whitespace issues, looks good to me -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4826#note_89923
participants (3)
-
Brendan Shanks (＠bshanks) -
Marc-Aurel Zent
-
Marc-Aurel Zent (＠mzent)