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

[Luca]

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.