svn commit: r192373 - head/sys/compat/linux

John Baldwin jhb at freebsd.org
Wed May 20 14:02:17 UTC 2009


On Tuesday 19 May 2009 4:19:56 pm Ben Kaduk wrote:
> On Tue, May 19, 2009 at 5:10 AM, Dmitry Chagin <dchagin at freebsd.org> wrote:
> > Author: dchagin
> > Date: Tue May 19 09:10:53 2009
> > New Revision: 192373
> > URL: http://svn.freebsd.org/changeset/base/192373
> >
> > Log:
> >  Validate user-supplied arguments values.
> >  Args argument is a pointer to the structure located in user space in
> >  which the socketcall arguments are packed. The structure must be
> >  copied to the kernel instead of direct dereferencing.
> >
> >  Approved by:  kib (mentor)
> >  MFC after:    1 week
> >
> > Modified:
> >  head/sys/compat/linux/linux_socket.c
> >
> > Modified: head/sys/compat/linux/linux_socket.c
> > 
==============================================================================
> > --- head/sys/compat/linux/linux_socket.c        Tue May 19 05:36:10 
2009        (r192372)
> > +++ head/sys/compat/linux/linux_socket.c        Tue May 19 09:10:53 
2009        (r192373)
> > @@ -1467,11 +1467,38 @@ linux_getsockopt(struct thread *td, stru
> >        return (error);
> >  }
> >
> > +/* Argument list sizes for linux_socketcall */
> > +
> > +#define LINUX_AL(x) ((x) * sizeof(l_ulong))
> > +
> > +static const unsigned char lxs_args[] = {
> > +       LINUX_AL(0) /* unused*/,        LINUX_AL(3) /* socket */,
> > +       LINUX_AL(3) /* bind */,         LINUX_AL(3) /* connect */,
> > +       LINUX_AL(2) /* listen */,       LINUX_AL(3) /* accept */,
> > +       LINUX_AL(3) /* getsockname */,  LINUX_AL(3) /* getpeername */,
> > +       LINUX_AL(4) /* socketpair */,   LINUX_AL(4) /* send */,
> > +       LINUX_AL(4) /* recv */,         LINUX_AL(6) /* sendto */,
> > +       LINUX_AL(6) /* recvfrom */,     LINUX_AL(2) /* shutdown */,
> > +       LINUX_AL(5) /* setsockopt */,   LINUX_AL(5) /* getsockopt */,
> > +       LINUX_AL(3) /* sendmsg */,      LINUX_AL(3) /* recvmsg */
> > +};
> > +
> > +#define        LINUX_AL_SIZE   sizeof(lxs_args) / sizeof(lxs_args[0]) - 1
> > +
> >  int
> >  linux_socketcall(struct thread *td, struct linux_socketcall_args *args)
> >  {
> > -       void *arg = (void *)(intptr_t)args->args;
> > +       l_ulong a[6];
> > +       void *arg;
> > +       int error;
> > +
> > +       if (args->what < LINUX_SOCKET || args->what > LINUX_AL_SIZE)
> > +               return (EINVAL);
> > +       error = copyin(PTRIN(args->args), a, lxs_args[args->what]);
> > +       if (error)
> > +               return (error);
> >
> > +       arg = a;
> >        switch (args->what) {
> >        case LINUX_SOCKET:
> >                return (linux_socket(td, arg));
> 
> 
> What factors go into deciding to do bounds-checking before the copyin versus
> after the copyin?  Naively, I would be worried about the userland data 
changing
> out from under the kernel, but I'm not terribly familiar with this area.

Well, the 'args->what' can't change out from under us as we only read it once.  
If a user app does change the memory args->args points to then it will merely 
get undefined userland behavior (so it may get an unexpected error because we 
interpret the arg structure based on the value of 'args->what' at the time of 
the initial copyin of the direct args from userland).  This case is very 
similar to an ioctl that passes a structure with a pointer to another 
structure.  The top-level structure (just as the top-level system call args) 
is only read once, so there is no chance for userland to change it after the 
kernel has already done validation (as there is with things like systrace 
(and why systrace is fundamentally broken, but that's Robert's line)).  
Similar guarantees can be made when handling sub-structures by only reading 
them once so that all the various access checks and operations are performed 
on the same data.  In this case the args->args data is only being read once, 
so it is fine.  ioctl handlers should also only read nested structures once 
in their entirety before using their contents.

-- 
John Baldwin


More information about the svn-src-head mailing list