bug-hurd
[Top][All Lists]
Advanced

[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: Samuel Thibault
Subject: Re: [PATCH 10/15] x86_64: expand and shrink messages in copy{in, out}msg routines
Date: Sun, 28 Aug 2022 15:13:54 +0200
User-agent: NeoMutt/20170609 (1.8.3)

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



reply via email to

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