Date: Wed, 20 May 2009 08:17:42 -0400 From: John Baldwin <jhb@freebsd.org> To: Ben Kaduk <minimarmot@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Dmitry Chagin <dchagin@freebsd.org> Subject: Re: svn commit: r192373 - head/sys/compat/linux Message-ID: <200905200817.43269.jhb@freebsd.org> In-Reply-To: <47d0403c0905191319w77c8849t5dca0b297b292a34@mail.gmail.com> References: <200905190910.n4J9Arvs090603@svn.freebsd.org> <47d0403c0905191319w77c8849t5dca0b297b292a34@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 19 May 2009 4:19:56 pm Ben Kaduk wrote: > On Tue, May 19, 2009 at 5:10 AM, Dmitry Chagin <dchagin@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200905200817.43269.jhb>