Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 26 Jan 2012 19:36:41 +0400
From:      Gleb Smirnoff <glebius@FreeBSD.org>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: svn commit: r230583 - head/sys/kern
Message-ID:  <20120126153641.GA68112@FreeBSD.org>
In-Reply-To: <20120126233110.C960@besplex.bde.org>
References:  <201201261159.q0QBxma2086162@svn.freebsd.org> <20120126233110.C960@besplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--Nq2Wo0NMKNjxTN9z
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline

  Bruce,

On Thu, Jan 26, 2012 at 11:53:57PM +1100, Bruce Evans wrote:
B> > Log:
B> >  Although aio_nbytes is size_t, later is is signed to
B> >  casted types: to ssize_t in filesystem code and to
B> >  int in buf code, thus supplying a negative argument
B> >  leads to kernel panic later.
B> 
B> And supplying a large positive argument leads to undefined
B> behaviour later in the buf case.
B> 
B> > To fix that check user
B> >  supplied argument in the beginning of syscall.
B> 
B> Now, supplying a large positive argument gives implementation-
B> defined behaviour immediately.  Sometimes the implementation-
B> defined behaviour is to have no effect immediately and allow
B> a panic later.
B> 
B> > Modified: head/sys/kern/vfs_aio.c
B> > ==============================================================================
B> > --- head/sys/kern/vfs_aio.c	Thu Jan 26 11:15:12 2012	(r230582)
B> > +++ head/sys/kern/vfs_aio.c	Thu Jan 26 11:59:48 2012	(r230583)
B> > @@ -1552,6 +1552,12 @@ aio_aqueue(struct thread *td, struct aio
B> > 		return (error);
B> > 	}
B> >
B> > +	/* XXX: aio_nbytes is later casted to signed types. */
B> > +	if ((int)aiocbe->uaiocb.aio_nbytes < 0) {
B> 
B> This should avoid implementation-defined behaviour by checking if
B> 
B>  	(uncast)aiocbe->uaiocb.aio_nbytes > INT_MAX.
B> 
B> It isn't clear what the effects of the implementation-defined behaviour
B> are even when you know what it is.  It certainly results in the
B> check passing values that exceed INT_MAX.  For example, with 32-bit
B> int and 64-bit size_t, and everything 2's complement, and no perverse
B> implementation-defined behaviour like "the result of converting to
B> int when the argment exceeds INT_MAX is always 42", then
B> (int)0x10000000 is 0.

Is the attached patch okay?

-- 
Totus tuus, Glebius.

--Nq2Wo0NMKNjxTN9z
Content-Type: text/x-diff; charset=koi8-r
Content-Disposition: attachment; filename="vfs_aio.c.diff"

Index: vfs_aio.c
===================================================================
--- vfs_aio.c	(revision 230585)
+++ vfs_aio.c	(working copy)
@@ -1553,7 +1553,7 @@
 	}
 
 	/* XXX: aio_nbytes is later casted to signed types. */
-	if ((int)aiocbe->uaiocb.aio_nbytes < 0) {
+	if (aiocbe->uaiocb.aio_nbytes > INT_MAX) {
 		uma_zfree(aiocb_zone, aiocbe);
 		return (EINVAL);
 	}

--Nq2Wo0NMKNjxTN9z--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120126153641.GA68112>