On Mon, Jan 30, 2012, Kostik Belousov wrote:
> On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote:
> > On Sun, Jan 29, 2012, Kostik Belousov wrote:
> > > On Sat, Jan 28, 2012 at 07:12:25PM -0500, David Schultz wrote:
> > > > On Sat, Jan 28, 2012, Kostik Belousov wrote:
> > > > > On Fri, Jan 27, 2012 at 02:42:21PM -0500, David Schultz wrote:
> > > > > > The correct limit on the maximum size of a single read/write is
> > > > > > SSIZE_MAX, but FreeBSD uses INT_MAX. It's not safe to raise the
> > > > > > limit yet, though, because of bugs in several filesystems. For
> > > > > > example, FFS copies uio_resid into a local variable of type int.
> > > > > > I have some old patches that fix some of these issues for FFS and
> > > > > > cd9660, but surely there are more places I didn't notice.
> > > > > >
> > > > > Absolutely agree.
> > > > >
> > > > > http://people.freebsd.org/~kib/misc/uio_resid.5.patch
> > > >
> > > > Nice. You found a lot more than I've got in my tree, and you even
> > > > fixed the return values. There are at least a few more places to
> > > > fix. For instance, cd9660 and the NFS client pass uio_resid or
> > > > iov_len to min(), which operates on ints. (Incidentally, C11
> > > > generics ought to make it possible to write type-generic min()
> > > > and max() functions.)
> > >
> > > Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
> > > changed them to MIN().
> >
> > This looks good to me. I tried to think of other places that you
> > might have missed, and the only one that occurred to me is the
> Might ? I think this is a blatant understate.
>
> > pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
> > casts of iov_len and uio_resid to type u_int. Some look harmless,
> > although it appears that writing a multiple of 2^32 bytes might
> > result in pipe_build_write_buffer() allocating a 0-length buffer.
> >
> > My only reservation is that raising the limit could unmask a
> > kernel buffer overflow if we missed something, but I guess we have
> > to cross that bridge some day anyway.
> Yes, and it is an obvious reason why I am chicken to commit this for
> so long time. One more place, if this is reasonable to count as 'one'
> place, are the cdevsw methods. devfs passes uio down to the drivers.
That's why I'm glad I'm not committing it. :) A more conservative
change (also known as "kicking the can down the road") would be to
add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems
that have been thoroughly reviewed. The VFS layer could cap the size
at INT_MAX for file systems without the flag.
> diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
> index 9edcb74..332ec37 100644
> --- a/sys/kern/sys_pipe.c
> +++ b/sys/kern/sys_pipe.c
[...]
> @@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio)
> struct pipe *wpipe;
> struct uio *uio;
> {
> - u_int size;
> + size_t size;
> int i;
>
> PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED);
> KASSERT(wpipe->pipe_state & PIPE_DIRECTW,
> ("Clone attempt on non-direct write pipe!"));
>
> - size = (u_int) uio->uio_iov->iov_len;
> + size = uio->uio_iov->iov_len;
> if (size > wpipe->pipe_buffer.size)
> size = wpipe->pipe_buffer.size;
The transfer can't be bigger than the max pipe buffer size (64k),
so `size = (int)MIN(uio->uio_iov->iov_len, wpipe->pipe_buffer.size)'
should suffice. The same comment applies elsewhere in the file.
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
