From owner-freebsd-hackers@FreeBSD.ORG Sun Sep 19 17:53:08 2004 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 51A1916A4CE; Sun, 19 Sep 2004 17:53:08 +0000 (GMT) Received: from rosebud.otenet.gr (rosebud.otenet.gr [195.170.0.26]) by mx1.FreeBSD.org (Postfix) with ESMTP id 71FD743D46; Sun, 19 Sep 2004 17:53:07 +0000 (GMT) (envelope-from keramida@freebsd.org) Received: from gothmog.gr (patr530-b122.otenet.gr [212.205.244.130]) i8JHr1BG019748; Sun, 19 Sep 2004 20:53:02 +0300 Received: from gothmog.gr (gothmog [127.0.0.1]) by gothmog.gr (8.13.1/8.13.1) with ESMTP id i8JHofeR076625; Sun, 19 Sep 2004 20:50:42 +0300 (EEST) (envelope-from keramida@freebsd.org) Received: (from giorgos@localhost) by gothmog.gr (8.13.1/8.13.1/Submit) id i8JGoBFq031925; Sun, 19 Sep 2004 19:50:11 +0300 (EEST) (envelope-from keramida@freebsd.org) Date: Sun, 19 Sep 2004 19:50:11 +0300 From: Giorgos Keramidas To: Pawel Jakub Dawidek , Don Lewis Message-ID: <20040919165011.GA2907@gothmog.gr> References: <20040918090227.GX30151@darkness.comp.waw.pl> <200409180918.i8I9ItWl001012@gw.catspoiler.org> <4146316C00007833@ims3a.cp.tin.it> <20040917093712.GB94990@orion.daedalusnetworks.priv> <20040918090227.GX30151@darkness.comp.waw.pl> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200409180918.i8I9ItWl001012@gw.catspoiler.org> <20040918090227.GX30151@darkness.comp.waw.pl> cc: freebsd-hackers@freebsd.org cc: gerarra@tin.it Subject: Re: FreeBSD Kernel buffer overflow X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 19 Sep 2004 17:53:08 -0000 On 2004-09-18 11:02, Pawel Jakub Dawidek wrote: > On Fri, Sep 17, 2004 at 12:37:12PM +0300, Giorgos Keramidas wrote: > +> % +#ifdef INVARIANTS > +> % + KASSERT(0 <= narg && narg <= 8, ("invalid number of syscall args")); > +> % +#endif > > Maybe: > KASSERT(0 <= narg && narg <= sizeof(args) / sizeof(args[0]), > ("invalid number of syscall args")); > > So if we decide to increase/decrease it someday, we don't have to remember > about this KASSERT(). This might actually be good to have for kernels with INVARIANTS regardless of the small penalty paid for every syscall. Debugging kernels are known to be slower than non-debugging ones. It won't come as a very big surprise for those who are really interested to turn it on. Is there some way we can measure the extra slowness of a kernel compiled with this KASSERT turned on? Then, if we find out that turning this on for all the developers who use INVARIANTS is too big a penalty to pay we remove it or add a special category of invariants (INVARIANTS_SLOW anyone?) that will act as a toggle for INVARIANTS that are known to cause extreme slowness and should only be enabled with care... On 2004-09-18 02:18, Don Lewis wrote: > If you think we really need this bit of extra security, why not just > prevent the syscall with too many arguments from being registered by > syscall_register()? At least that keeps the check out of the most > frequently executed path. Don, This sounds excellent. Can an src-committer verify that the following is ok and commit it along with the manpage diff I posted earlier to HEAD? The hard-wired number 8 in there seems like something that could probably be improved a lot, but after looking for a short while I couldn't find a good way of finding out from the arguments of syscall_register() some way to calculate it. Of course, I'm far from an experienced kernel hacker and I'm probably missing something. Feel free to correct the following diff or even replace it entirely. : Index: kern_syscalls.c : =================================================================== : RCS file: /home/ncvs/src/sys/kern/kern_syscalls.c,v : retrieving revision 1.11 : diff -u -r1.11 kern_syscalls.c : --- kern_syscalls.c 15 Jul 2004 08:26:05 -0000 1.11 : +++ kern_syscalls.c 19 Sep 2004 16:38:21 -0000 : @@ -58,6 +58,8 @@ : syscall_register(int *offset, struct sysent *new_sysent, : struct sysent *old_sysent) : { : + if (new_sysent->sy_narg < 0 || new_sysent->sy_narg > 8) : + return EINVAL; : if (*offset == NO_SYSCALL) { : int i; : P.S. I noticed that the kern_syscall.c file has many whitespace ``issues'' (i.e. it uses 7 SPACE characters for the first level of indentation in syscall_register() but 8 SPACES for the deeper levels of nesting), which should probably be taken care of in future commits, but that's really a different another topic. - Giorgos