From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753714AbZGVNjA (ORCPT ); Wed, 22 Jul 2009 09:39:00 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752350AbZGVNi7 (ORCPT ); Wed, 22 Jul 2009 09:38:59 -0400 Received: from mx2.redhat.com ([66.187.237.31]:35895 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751209AbZGVNi7 (ORCPT ); Wed, 22 Jul 2009 09:38:59 -0400 Subject: Re: [PATCH] fix offset checks in do_sendfile to use unsigned values From: Jeff Layton To: Johannes Weiner Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, smfrench@gmail.com, hch@infradead.org In-Reply-To: <20090722125945.GA2834@cmpxchg.org> References: <1248262099-12514-1-git-send-email-jlayton@redhat.com> <1248262099-12514-2-git-send-email-jlayton@redhat.com> <20090722125945.GA2834@cmpxchg.org> Content-Type: text/plain Date: Wed, 22 Jul 2009 09:37:59 -0400 Message-Id: <1248269879.4534.12.camel@tlielax.poochiereds.net> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > > 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