Date: Sun, 19 Sep 2004 19:50:11 +0300 From: Giorgos Keramidas <keramida@freebsd.org> To: Pawel Jakub Dawidek <pjd@freebsd.org>, Don Lewis <truckman@freebsd.org> Cc: gerarra@tin.it Subject: Re: FreeBSD Kernel buffer overflow Message-ID: <20040919165011.GA2907@gothmog.gr> In-Reply-To: <200409180918.i8I9ItWl001012@gw.catspoiler.org> <20040918090227.GX30151@darkness.comp.waw.pl> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2004-09-18 11:02, Pawel Jakub Dawidek <pjd@freebsd.org> 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 <truckman@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040919165011.GA2907>