[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 10/15] x86_64: expand and shrink messages in copy{in, out}msg

From: Luca
Subject: Re: [PATCH 10/15] x86_64: expand and shrink messages in copy{in, out}msg routines
Date: Tue, 30 Aug 2022 07:57:23 +0200

Il 28/08/22 15:13, Samuel Thibault ha scritto:
That's great work :D

Thanks :)

Luca Dariz, le mar. 28 juin 2022 12:10:49 +0200, a ecrit:
diff --git a/i386/i386/copy_user.h b/i386/i386/copy_user.h
new file mode 100644
index 00000000..ab932401
--- /dev/null
+++ b/i386/i386/copy_user.h
@@ -0,0 +1,22 @@

Please add the copyright header.

Will do

+#ifndef COPY_USER_H
+#define COPY_USER_H
+#include <sys/types.h>
+#include <machine/locore.h>
+#include <mach/message.h>

diff --git a/ipc/ipc_kmsg.c b/ipc/ipc_kmsg.c
index 09801924..8f7045ee 100644
--- a/ipc/ipc_kmsg.c
+++ b/ipc/ipc_kmsg.c
@@ -529,7 +530,6 @@ ipc_kmsg_get(
                return MACH_SEND_INVALID_DATA;
- kmsg->ikm_header.msgh_size = size;
        *kmsgp = kmsg;
        return MACH_MSG_SUCCESS;

This was breaking the 32bit kernel case. I have pushed a fix for that,
that does this move of setting msgh_size to copyinmsg itself.

The 32-bit case was breaking because it needed an updated MIG, I should have sent a patch for that too. But probably maintaining backward compatibility would have been better, thanks for fixing that.

It's a bit redundant to have the message size in two places when sending, but for the shrink/expand required by ports it was simpler to use the inline parameter, and more consistent with the rx path.

In the future maybe we could add a mach_msg2() syscall with a different signature, and maybe add some other improvements if needed (I didn't find anything in particular yet). MIG could then generate compatible code depending on the gnumach headers.

@@ -1393,7 +1393,19 @@ ipc_kmsg_copyin_body(
                                if (data == 0)
                                        goto invalid_memory;
- if (copyinmap(map, (char *) addr,
+                               if (sizeof(mach_port_name_t) != 
+                               {
+                                       mach_port_name_t *src = 
+                                       mach_port_t *dst = (mach_port_t*)data;
+                                       for (int i=0; i<number; i++)
+                                               *(dst + i) = *(src + i);

We shall not dereference user pointers like that :) If userland provided
bogus pointers, we'd here either kernel_trap, or worse: erroneously use
kernel pointer that nasty userland passed on.  All along all such code,
we have to use copyin/copyout each time (just only get_user/put_user in
Linux), and check their result to possibly return an address error.

Right, I forgot this part directly accessed userspace. I'll add copyin/copyout wrappers.

As far as I understand, these routines should use stac/clac if the SMAP cpu feature is supported on x86 as the Linux counterparts, so we would catch these cases earlier.

I didn't find anything related to cpu features yet, is it something still to be addressed? Is there a minimum that we can assume to have? (maybe ensuring at early boot that the cpu supports the features)

diff --git a/x86_64/copy_user.c b/x86_64/copy_user.c
new file mode 100644
index 00000000..b9c94d76
--- /dev/null
+++ b/x86_64/copy_user.c
@@ -0,0 +1,280 @@

This is also missing the copyright header.

Will add it.

+#include <string.h>
+#include <kern/debug.h>
+#include <mach/boolean.h>
+#include <copy_user.h>

+size_t msg_usize(const mach_msg_header_t *kmsg)
+          if (is_inline)
+            {
+              if (MACH_MSG_TYPE_PORT_ANY(name))
+                {
+                  saddr += 8 * number;
+                  usize += 4 * number;

No magic number please :)
This should be sizeof(port), sizeof(port_name), etc.

Will do.

+                }
+              else
+                {
+                  size_t n = size / 8;
+                  saddr += n*number;
+                  usize += n*number;
+                  align_inline(saddr, 4);
+                  align_inline(usize, 4);

This should be alignof(some_align_type)

Will do.

+                }
+            }
+          else
+            {
+              // advance one pointer
+              saddr += 8;
+              usize += 4;

This should be sizeof(kptr), sizeof(uptr).

And similarly in the rest of file.

Will do.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]