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> wrot= e: > > Author: dchagin > > Date: Tue May 19 09:10:53 2009 > > New Revision: 192373 > > URL: http://svn.freebsd.org/changeset/base/192373 > > > > Log: > > =A0Validate user-supplied arguments values. > > =A0Args argument is a pointer to the structure located in user space in > > =A0which the socketcall arguments are packed. The structure must be > > =A0copied to the kernel instead of direct dereferencing. > > > > =A0Approved by: =A0kib (mentor) > > =A0MFC after: =A0 =A01 week > > > > Modified: > > =A0head/sys/compat/linux/linux_socket.c > > > > Modified: head/sys/compat/linux/linux_socket.c > >=20 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > > --- head/sys/compat/linux/linux_socket.c =A0 =A0 =A0 =A0Tue May 19 05:3= 6:10=20 2009 =A0 =A0 =A0 =A0(r192372) > > +++ head/sys/compat/linux/linux_socket.c =A0 =A0 =A0 =A0Tue May 19 09:1= 0:53=20 2009 =A0 =A0 =A0 =A0(r192373) > > @@ -1467,11 +1467,38 @@ linux_getsockopt(struct thread *td, stru > > =A0 =A0 =A0 =A0return (error); > > =A0} > > > > +/* Argument list sizes for linux_socketcall */ > > + > > +#define LINUX_AL(x) ((x) * sizeof(l_ulong)) > > + > > +static const unsigned char lxs_args[] =3D { > > + =A0 =A0 =A0 LINUX_AL(0) /* unused*/, =A0 =A0 =A0 =A0LINUX_AL(3) /* so= cket */, > > + =A0 =A0 =A0 LINUX_AL(3) /* bind */, =A0 =A0 =A0 =A0 LINUX_AL(3) /* co= nnect */, > > + =A0 =A0 =A0 LINUX_AL(2) /* listen */, =A0 =A0 =A0 LINUX_AL(3) /* acce= pt */, > > + =A0 =A0 =A0 LINUX_AL(3) /* getsockname */, =A0LINUX_AL(3) /* getpeern= ame */, > > + =A0 =A0 =A0 LINUX_AL(4) /* socketpair */, =A0 LINUX_AL(4) /* send */, > > + =A0 =A0 =A0 LINUX_AL(4) /* recv */, =A0 =A0 =A0 =A0 LINUX_AL(6) /* se= ndto */, > > + =A0 =A0 =A0 LINUX_AL(6) /* recvfrom */, =A0 =A0 LINUX_AL(2) /* shutdo= wn */, > > + =A0 =A0 =A0 LINUX_AL(5) /* setsockopt */, =A0 LINUX_AL(5) /* getsocko= pt */, > > + =A0 =A0 =A0 LINUX_AL(3) /* sendmsg */, =A0 =A0 =A0LINUX_AL(3) /* recv= msg */ > > +}; > > + > > +#define =A0 =A0 =A0 =A0LINUX_AL_SIZE =A0 sizeof(lxs_args) / sizeof(lxs= _args[0]) - 1 > > + > > =A0int > > =A0linux_socketcall(struct thread *td, struct linux_socketcall_args *ar= gs) > > =A0{ > > - =A0 =A0 =A0 void *arg =3D (void *)(intptr_t)args->args; > > + =A0 =A0 =A0 l_ulong a[6]; > > + =A0 =A0 =A0 void *arg; > > + =A0 =A0 =A0 int error; > > + > > + =A0 =A0 =A0 if (args->what < LINUX_SOCKET || args->what > LINUX_AL_SI= ZE) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return (EINVAL); > > + =A0 =A0 =A0 error =3D copyin(PTRIN(args->args), a, lxs_args[args->wha= t]); > > + =A0 =A0 =A0 if (error) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return (error); > > > > + =A0 =A0 =A0 arg =3D a; > > =A0 =A0 =A0 =A0switch (args->what) { > > =A0 =A0 =A0 =A0case LINUX_SOCKET: > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (linux_socket(td, arg)); >=20 >=20 > What factors go into deciding to do bounds-checking before the copyin ver= sus > after the copyin? Naively, I would be worried about the userland data=20 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 on= ce. =20 If a user app does change the memory args->args points to then it will mere= ly=20 get undefined userland behavior (so it may get an unexpected error because = we=20 interpret the arg structure based on the value of 'args->what' at the time = of=20 the initial copyin of the direct args from userland). This case is very=20 similar to an ioctl that passes a structure with a pointer to another=20 structure. The top-level structure (just as the top-level system call args= )=20 is only read once, so there is no chance for userland to change it after th= e=20 kernel has already done validation (as there is with things like systrace=20 (and why systrace is fundamentally broken, but that's Robert's line)). =20 Similar guarantees can be made when handling sub-structures by only reading= =20 them once so that all the various access checks and operations are performe= d=20 on the same data. In this case the args->args data is only being read once= ,=20 so it is fine. ioctl handlers should also only read nested structures once= =20 in their entirety before using their contents. =2D-=20 John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200905200817.43269.jhb>