[PATCH v4 0/3] MR4826: Mach vm operations improvements

[Marc-Aurel Zent (＠mzent)]

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

--
  v4: server: Simplify mach write_process_memory.
      server: Use mach_vm_read_overwrite in get_selector_entry.
      server: Use mach_vm_read_overwrite in read_process_memory.

https://gitlab.winehq.org/wine/wine/-/merge_requests/4826

[PATCH v4 1/3] server: Use mach_vm_read_overwrite in read_process_memory.

[Marc-Aurel Zent]

From: Marc-Aurel Zent

---
 server/mach.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/server/mach.c b/server/mach.c
index 86ff7eac382..aa72a9f8086 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,24 +406,8 @@ 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;
-
-    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 );
-    }
-    task_resume( process_port );
+    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);
 }

[PATCH v4 2/3] server: Use mach_vm_read_overwrite in get_selector_entry.

[Marc-Aurel Zent]

From: Marc-Aurel Zent

---
 server/mach.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/server/mach.c b/server/mach.c
index aa72a9f8086..5aa37d4c93c 100644
--- a/server/mach.c
+++ b/server/mach.c
@@ -506,9 +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)
@@ -518,29 +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;
     }

-    if ((ret = task_suspend( process_port )) == KERN_SUCCESS)
+    if (!(data = (vm_offset_t)malloc( total_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;
+        set_error( STATUS_NO_MEMORY );
+        return;
+    }

-        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_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 *)data;
+        memcpy( base, ldt + entry, sizeof(int) );
+        memcpy( limit, ldt + entry + 8192, sizeof(int) );
+        memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 );
     }
-    else mach_set_error( ret );
+
+    free( data );
 }

#endif  /* USE_MACH */

[PATCH v4 3/3] server: Simplify mach write_process_memory.

[Marc-Aurel Zent]

From: Marc-Aurel Zent

---
 server/mach.c | 72 ++++++---------------------------------------------
 1 file changed, 8 insertions(+), 64 deletions(-)

diff --git a/server/mach.c b/server/mach.c
index 5aa37d4c93c..f1073835e00 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,68 +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;
-
-    if ((ret = task_suspend( process_port )) != KERN_SUCCESS)
+    if (posix_memalign( &data, page_size, size ))
     {
-        mach_set_error( ret );
+        set_error( STATUS_NO_MEMORY );
         return 0;
     }

-    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, &region_address, &region_size, VM_REGION_BASIC_INFO_64,
-                     (vm_region_info_t)&info, &info_size, &dummy );
-    if (ret != KERN_SUCCESS)
-    {
-        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;
-    }
-
-    /* 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 );
-
-    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 );
-        task_resume( process_port );
-        return 1;
-    }
+    memcpy( data, src, size );

-failed:
-    if (task_mem) mach_vm_deallocate( mach_task_self(), task_mem, bytes_read );
-    task_resume( process_port );
-    return 0;
+    ret = mach_vm_write( process_port, (mach_vm_address_t)ptr, data, (mach_msg_type_number_t)size );
+    free( data );
+    mach_set_error( ret );
+    return (ret == KERN_SUCCESS);
 }

/* retrieve an LDT selector entry */

[Marc-Aurel Zent (＠mzent)]

Should be rebased now, are there any concerns/blockers with this?