Re: [PATCH] fix offset checks in do_sendfile to use unsigned values

[Jeff Layton]

On Wed, 2009-07-22 at 14:59 +0200, Johannes Weiner wrote:
> On Wed, Jul 22, 2009 at 07:28:19AM -0400, Jeff Layton wrote:
> > If do_sendfile is called with a "max" value of 0, it grabs the lesser
> > s_maxbytes value of the two superblocks to use instead. There's a
> > problem here however. s_maxbytes is an unsigned long long and it gets
> > cast to a signed value. If both s_maxbytes values are large enough, max
> > will end up being negative and the comparisons in this code won't work
> > correctly.
> > 
> > Change do_sendfile to use unsigned values internally for the offset
> > checks.
> > 
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> >  fs/read_write.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 6c8c55d..36899ff 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -788,11 +788,11 @@ SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
> >  }
> >  
> >  static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
> > -			   size_t count, loff_t max)
> > +			   size_t count, unsigned long long max)
> >  {
> >  	struct file * in_file, * out_file;
> >  	struct inode * in_inode, * out_inode;
> > -	loff_t pos;
> > +	unsigned long long pos;
> >  	ssize_t retval;
> >  	int fput_needed_in, fput_needed_out, fl;
> >  
> > @@ -838,7 +838,7 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
> >  	if (!max)
> >  		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
> >  
> > -	pos = *ppos;
> > +	pos = (unsigned long long) *ppos;
> >  	retval = -EINVAL;
> >  	if (unlikely(pos < 0))
> >  		goto fput_out;
> 
> May it be possible that cifs is the only fs that sets sb->sb_maxbytes
> to exceed loff_t?  It seems the others clamp it to MAX_LFS_FILESIZE
> while CIFS exceeds this by one.  And then max is -1.
> 
> So, isn't the correct fix something similar to this?
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index e16d759..df56093 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2452,10 +2452,10 @@ try_mount_again:
>  		tcon->local_lease = volume_info->local_lease;
>  	}
>  	if (pSesInfo) {
> -		if (pSesInfo->capabilities & CAP_LARGE_FILES) {
> -			sb->s_maxbytes = (u64) 1 << 63;
> -		} else
> -			sb->s_maxbytes = (u64) 1 << 31;	/* 2 GB */
> +		if (pSesInfo->capabilities & CAP_LARGE_FILES)
> +			sb->s_maxbytes = MAX_LFS_FILESIZE;
> +		else
> +			sb->s_maxbytes = MAX_NON_LFS;
>  	}
>  
>  	/* BB FIXME fix time_gran to be larger for LANMAN sessions */

Yes and I posted that exact same cifs patch yesterday. I think we also
need to do a similar fix for get_sb_pseudo. It currently sets s_maxbytes
to ~0ULL...

Any of these patches will fix the immediate problem, but I think this
code in do_sendfile should still account for the possibility that
someone can set the value larger than MAX_LFS_FILESIZE. An alternative
is to consider a WARN at mount time when filesystems set s_maxbytes
larger than that value (that might help catch out of tree filesystems
that get this wrong and prevent this sort of silent bug in the future).

Either way, the patch I posted for this isn't sufficient since there are
some checks that need to be done against the signed values (the
(pos < 0) check, for instance). I'll post a respun patch in a bit that
should fix up those problems.

Thanks,
-- 
Jeff Layton <jlayton@xxxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/