Hello,
> diff --git a/gnumach/ipc/ipc_kmsg.c b/gnumach/ipc/ipc_kmsg.c
> index 8ab0e2e..19a76f3 100644
> --- a/gnumach/ipc/ipc_kmsg.c
> +++ b/gnumach/ipc/ipc_kmsg.c
> @@ -450,14 +450,21 @@ ipc_kmsg_clean_partial(
> void
> ipc_kmsg_free(ipc_kmsg_t kmsg)
> {
> - vm_size_t size = kmsg->ikm_size;
> + vm_size_t size;
> +
> + /* Early exit: NULL kmsg protection */
> + if (unlikely(kmsg == IKM_NULL))
> + return;
No, we don't want to silently ignore this. An ipc_kmsg_free caller is
not supposed to pass it a null pointer. Ignoring this means hiding an
more profound issue. And hiding bugs is the worse.
> switch (size) {
> case IKM_SIZE_NETWORK:
> net_kmsg_put(kmsg);
> break;
>
> default:
> + if (unlikely(size == 0 || size > IKM_SAVED_MAX))
> + return;
Again, hiding bugs (and kfree already checks for 0)
> kfree((vm_offset_t) kmsg, size);
> break;
> }
> }
> @@ -485,6 +492,11 @@ ipc_kmsg_get(
> if ((size < sizeof(mach_msg_user_header_t)) ||
> mach_msg_user_is_misaligned(size))
> return MACH_SEND_MSG_TOO_SMALL;
>
> + /* Bounds check & integer overflow protection */
> + if (unlikely(size > IKM_SAVED_MAX ||
What is that constant?
> + size > (MACH_MSG_SIZE_MAX / IKM_EXPAND_FACTOR)))
That does not really protect the kernel, user would still be able to
give a *very* large value.
> + return MACH_SEND_NO_BUFFER;
> +
> if (ksize <= IKM_SAVED_MSG_SIZE) {
> kmsg = ikm_cache_alloc();
> if (kmsg == IKM_NULL)
> @@ -497,7 +509,7 @@ ipc_kmsg_get(
> }
>
> if (copyinmsg(msg, &kmsg->ikm_header, size, kmsg->ikm_size)) {
> - ikm_free(kmsg);
> + ipc_kmsg_free(kmsg);
Why? It's the same.
Samuel
