[PATCH] libfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[PATCH] libfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[Jeff Layton]

This patch is a counterpart to the CIFS patch posted yesterday. I
believe either patch will fix the problem seen in do_sendfile since the
smaller s_maxbytes value for the two superblocks is used there.

get_sb_pseudo sets s_maxbytes to ~0ULL, which becomes negative when cast
to a signed value. Fix it to use MAX_LFS_FILESIZE which casts properly
to a positive signed value.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/libfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index ddfa899..dcec3d3 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -217,7 +217,7 @@ int get_sb_pseudo(struct file_system_type *fs_type, char *name,
 		return PTR_ERR(s);
 
 	s->s_flags = MS_NOUSER;
-	s->s_maxbytes = ~0ULL;
+	s->s_maxbytes = MAX_LFS_FILESIZE;
 	s->s_blocksize = PAGE_SIZE;
 	s->s_blocksize_bits = PAGE_SHIFT;
 	s->s_magic = magic;
-- 
1.6.0.6

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

[Jeff Layton]

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@redhat.com>
---
 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;
-- 
1.6.0.6

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

[Johannes Weiner]

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@redhat.com>
> ---
>  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 */

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@redhat.com>
> > ---
> >  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@redhat.com>

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

[Johannes Weiner]

On Wed, Jul 22, 2009 at 09:37:59AM -0400, Jeff Layton wrote:
> 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@redhat.com>
> > > ---
> > >  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.

Sorry, I missed it.

> I think we also need to do a similar fix for get_sb_pseudo. It
> currently sets s_maxbytes to ~0ULL...

Yes, I saw it and agree with it.

> 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).

Isn't MAX_LFS_FILESIZE by definition the maximum sensible value for
s_maxbytes?

> 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.

That is already handled in rw_verify_area(), I think, so we should be
able to drop it completely.

	Hannes

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

[Jeff Layton]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On Wed, 2009-07-22 at 15:51 +0200, Johannes Weiner wrote:

> > 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).
> 
> Isn't MAX_LFS_FILESIZE by definition the maximum sensible value for
> s_maxbytes?
> 

Pretty much, but nothing seems to enforce it or let you know when you've
exceeded it. It sort of seems like s_maxbytes ought to be loff_t or
something instead of an unsigned long long. A negative value there
wouldn't make much sense, but no one would be as tempted to set it
higher than MAX_LFS_FILESIZE.

> > 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.
> 
> That is already handled in rw_verify_area(), I think, so we should be
> able to drop it completely.

If we get rid of those checks altogether, then "max" will become unused.
Is that really OK here?

For discussion purposes, I've attached a replacement patch that I'm
working with now.
-- 
Jeff Layton <jlayton@redhat.com>

[-- Attachment #2: 0001-fix-offset-checks-in-do_sendfile-to-use-unsigned-val.patch --]
[-- Type: text/x-patch, Size: 2475 bytes --]

>From 00a22f2f1e34ba0765ca49120499e681477a265a Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Wed, 22 Jul 2009 08:36:22 -0400
Subject: [PATCH] fix offset checks in do_sendfile to use unsigned values (try #2)

This is the second version of this patch. Some of the checks do need
to use signed values. This patch should be more correct in that regard.
This also adds a check to make sure that "pos + count" doesn't
overflow.

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 against "max".

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/read_write.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 6c8c55d..2c5b402 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, newpos;
 	ssize_t retval;
 	int fput_needed_in, fput_needed_out, fl;
 
@@ -835,14 +835,16 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		goto fput_out;
 	count = retval;
 
-	if (!max)
-		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
-
-	pos = *ppos;
 	retval = -EINVAL;
-	if (unlikely(pos < 0))
+	if (unlikely(*ppos < 0))
 		goto fput_out;
-	if (unlikely(pos + count > max)) {
+
+	if (!max)
+		max = min(in_inode->i_sb->s_maxbytes,
+			  out_inode->i_sb->s_maxbytes);
+	pos = (unsigned long long) *ppos;
+	newpos = pos + count;
+	if (unlikely(newpos > max || newpos < count)) {
 		retval = -EOVERFLOW;
 		if (pos >= max)
 			goto fput_out;
@@ -869,7 +871,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 
 	inc_syscr(current);
 	inc_syscw(current);
-	if (*ppos > max)
+	pos = (unsigned long long) *ppos;
+	if (pos > max)
 		retval = -EOVERFLOW;
 
 fput_out:
-- 
1.6.2.5

Re: [PATCH] libfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[Steve French]

Ack

On Wed, Jul 22, 2009 at 6:28 AM, Jeff Layton<jlayton@redhat.com> wrote:
> This patch is a counterpart to the CIFS patch posted yesterday. I
> believe either patch will fix the problem seen in do_sendfile since the
> smaller s_maxbytes value for the two superblocks is used there.
>
> get_sb_pseudo sets s_maxbytes to ~0ULL, which becomes negative when cast
> to a signed value. Fix it to use MAX_LFS_FILESIZE which casts properly
> to a positive signed value.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/libfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index ddfa899..dcec3d3 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -217,7 +217,7 @@ int get_sb_pseudo(struct file_system_type *fs_type, char *name,
>                return PTR_ERR(s);
>
>        s->s_flags = MS_NOUSER;
> -       s->s_maxbytes = ~0ULL;
> +       s->s_maxbytes = MAX_LFS_FILESIZE;
>        s->s_blocksize = PAGE_SIZE;
>        s->s_blocksize_bits = PAGE_SHIFT;
>        s->s_magic = magic;
> --
> 1.6.0.6
>
>



-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Steve French]

On Wed, Jul 22, 2009 at 9:13 AM, Jeff Layton<jlayton@redhat.com> wrote:
> On Wed, 2009-07-22 at 15:51 +0200, Johannes Weiner wrote:
>
>> > 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).
>>
>> Isn't MAX_LFS_FILESIZE by definition the maximum sensible value for
>> s_maxbytes?
>>
>
> Pretty much, but nothing seems to enforce it or let you know when you've
> exceeded it. It sort of seems like s_maxbytes ought to be loff_t or
> something instead of an unsigned long long. A negative value there
> wouldn't make much sense, but no one would be as tempted to set it
> higher than MAX_LFS_FILESIZE.
>
>> > 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.
>>
>> That is already handled in rw_verify_area(), I think, so we should be
>> able to drop it completely.
>
> If we get rid of those checks altogether, then "max" will become unused.
> Is that really OK here?
>
> For discussion purposes, I've attached a replacement patch that I'm
> working with now.

Looks fine to me


-- 
Thanks,

Steve

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

[Johannes Weiner]

On Wed, Jul 22, 2009 at 10:13:52AM -0400, Jeff Layton wrote:
> On Wed, 2009-07-22 at 15:51 +0200, Johannes Weiner wrote:
> 
> > > 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).
> > 
> > Isn't MAX_LFS_FILESIZE by definition the maximum sensible value for
> > s_maxbytes?
> > 
> 
> Pretty much, but nothing seems to enforce it or let you know when you've
> exceeded it.

Kernel code shoots from the hips.  We're faster that way.  We might
shoot granny in the process.

> > > 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.
> > 
> > That is already handled in rw_verify_area(), I think, so we should be
> > able to drop it completely.
> 
> If we get rid of those checks altogether, then "max" will become unused.
> Is that really OK here?

We still need to check for exceeding s_maxbytes, the other checks are
redundant.

> For discussion purposes, I've attached a replacement patch that I'm
> working with now.

> >>From 00a22f2f1e34ba0765ca49120499e681477a265a Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton@redhat.com>
> Date: Wed, 22 Jul 2009 08:36:22 -0400
> Subject: [PATCH] fix offset checks in do_sendfile to use unsigned values (try #2)
> 
> This is the second version of this patch. Some of the checks do need
> to use signed values. This patch should be more correct in that regard.
> This also adds a check to make sure that "pos + count" doesn't
> overflow.
> 
> 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 against "max".
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/read_write.c |   21 ++++++++++++---------
>  1 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 6c8c55d..2c5b402 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, newpos;
>  	ssize_t retval;
>  	int fput_needed_in, fput_needed_out, fl;
>  
> @@ -835,14 +835,16 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  		goto fput_out;
>  	count = retval;
>  
> -	if (!max)
> -		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
> -
> -	pos = *ppos;
>  	retval = -EINVAL;
> -	if (unlikely(pos < 0))
> +	if (unlikely(*ppos < 0))
>  		goto fput_out;

That check is done in rw_verify_area().

> -	if (unlikely(pos + count > max)) {
> +
> +	if (!max)
> +		max = min(in_inode->i_sb->s_maxbytes,
> +			  out_inode->i_sb->s_maxbytes);
> +	pos = (unsigned long long) *ppos;
> +	newpos = pos + count;
> +	if (unlikely(newpos > max || newpos < count)) {

pos + count overflow is checked in rw_verify_area() as well.

>  		retval = -EOVERFLOW;
>  		if (pos >= max)
>  			goto fput_out;
> @@ -869,7 +871,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  
>  	inc_syscr(current);
>  	inc_syscw(current);
> -	if (*ppos > max)
> +	pos = (unsigned long long) *ppos;
> +	if (pos > max)
>  		retval = -EOVERFLOW;

This one is needed.

But frankly, I really don't like the approach of catching a bogus
s_maxbytes in do_sendfile().  If we want to sanity check s_maxbytes,
we should do it at mount time.

And allowing for a bigger s_maxbytes makes no sense if the data types
involved when accessing the file can not handle these offsets at all,
no?

	Hannes