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) !=
sizeof(mach_port_t))
+ {
+ mach_port_name_t *src =
(mach_port_name_t*)addr;
+ 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.
