Hello,
That's great work :D
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.
> +#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.
> @@ -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.
> 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.
> +#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.
> + }
> + 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)
> + }
> + }
> + else
> + {
> + // advance one pointer
> + saddr += 8;
> + usize += 4;
This should be sizeof(kptr), sizeof(uptr).
And similarly in the rest of file.
Samuel
