From owner-svn-src-all@FreeBSD.ORG Wed May 20 14:02:17 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EA84D106564A; Wed, 20 May 2009 14:02:16 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id AE8ED8FC12; Wed, 20 May 2009 14:02:16 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 59E9C46B2A; Wed, 20 May 2009 10:02:16 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 240898A025; Wed, 20 May 2009 10:02:15 -0400 (EDT) From: John Baldwin To: Ben Kaduk Date: Wed, 20 May 2009 08:17:42 -0400 User-Agent: KMail/1.9.7 References: <200905190910.n4J9Arvs090603@svn.freebsd.org> <47d0403c0905191319w77c8849t5dca0b297b292a34@mail.gmail.com> In-Reply-To: <47d0403c0905191319w77c8849t5dca0b297b292a34@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200905200817.43269.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Wed, 20 May 2009 10:02:15 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Dmitry Chagin Subject: Re: svn commit: r192373 - head/sys/compat/linux X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 May 2009 14:02:17 -0000 On Tuesday 19 May 2009 4:19:56 pm Ben Kaduk wrote: > On Tue, May 19, 2009 at 5:10 AM, Dmitry Chagin 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