2005-11-03 17:56:44

by Herbert Poetzl

[permalink] [raw]
Subject: do_sendfile ppos check ...


Hi Andrew!

friend of mine stumbled over the following issue:

do_sendfile() does an overflow check near the end, like this:

if (*ppos > max)
retval = -EOVERFLOW;

now both sys_sendfile and sys_sendfile64 do call do_sendfile()
similar to this:

if (offset) {
...
ret = do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
return ret;
}
return do_sendfile(out_fd, in_fd, NULL, count, 0);

which passes ppos as NULL, which in turn leads to an oops ...

here is a patch (suggestion) to handle this properly, which
also adjusts the max for sys_sendfile()
(let me know what you think!)


--- linux-2.6.14/fs/read_write.c 2005-10-28 20:49:45 +0200
+++ linux-2.6.14-sendfile/fs/read_write.c 2005-11-03 18:48:37 +0100
@@ -731,7 +731,8 @@ asmlinkage ssize_t sys_sendfile(int out_
return ret;
}

- return do_sendfile(out_fd, in_fd, NULL, count, 0);
+ pos = 0;
+ return do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
}

asmlinkage ssize_t sys_sendfile64(int out_fd, int in_fd, loff_t __user *offset, size_t count)
@@ -748,5 +749,6 @@ asmlinkage ssize_t sys_sendfile64(int ou
return ret;
}

- return do_sendfile(out_fd, in_fd, NULL, count, 0);
+ pos = 0;
+ return do_sendfile(out_fd, in_fd, &pos, count, 0);
}


2005-11-04 02:37:14

by Herbert Xu

[permalink] [raw]
Subject: Re: do_sendfile ppos check ...

Herbert Poetzl <[email protected]> wrote:
>
> which passes ppos as NULL, which in turn leads to an oops ...

But do_sendfile will set ppos to &in_file->f_pos if it's NULL.
Why isn't that working?

> @@ -731,7 +731,8 @@ asmlinkage ssize_t sys_sendfile(int out_
> return ret;
> }
>
> - return do_sendfile(out_fd, in_fd, NULL, count, 0);
> + pos = 0;
> + return do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
> }

The last argument is meant to be zero if you check the history.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2005-11-04 03:10:16

by Herbert Poetzl

[permalink] [raw]
Subject: Re: do_sendfile ppos check ...

On Fri, Nov 04, 2005 at 01:36:36PM +1100, Herbert Xu wrote:
> Herbert Poetzl <[email protected]> wrote:
> >
> > which passes ppos as NULL, which in turn leads to an oops ...
>
> But do_sendfile will set ppos to &in_file->f_pos if it's NULL.
> Why isn't that working?
>
> > @@ -731,7 +731,8 @@ asmlinkage ssize_t sys_sendfile(int out_
> > return ret;
> > }
> >
> > - return do_sendfile(out_fd, in_fd, NULL, count, 0);
> > + pos = 0;
> > + return do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
> > }
>
> The last argument is meant to be zero if you check the history.

hmm, why a different max than with offset?

currently investigating ... probably a removal of
the 'unnecessary' check (*ppos) would be a better
approach, something like:

--- linux-2.6/fs/read_write.c 2005-10-28 23:59:02.000000000 +0200
+++ linux-2.6/fs/read_write.c 2005-11-03 17:28:50.000000000 +0100
@@ -719,9 +719,6 @@
current->syscr++;
current->syscw++;

- if (*ppos > max)
- retval = -EOVERFLOW;
-
fput_out:
ds, fput_light(out_file, fput_needed_out);
fput_in:

thanks for the input,
Herbert

>
> Cheers,
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2005-11-04 03:17:03

by Herbert Xu

[permalink] [raw]
Subject: Re: do_sendfile ppos check ...

On Fri, Nov 04, 2005 at 04:10:13AM +0100, Herbert Poetzl wrote:
>
> currently investigating ... probably a removal of
> the 'unnecessary' check (*ppos) would be a better
> approach, something like:
>
> --- linux-2.6/fs/read_write.c 2005-10-28 23:59:02.000000000 +0200
> +++ linux-2.6/fs/read_write.c 2005-11-03 17:28:50.000000000 +0100
> @@ -719,9 +719,6 @@
> current->syscr++;
> current->syscw++;
>
> - if (*ppos > max)
> - retval = -EOVERFLOW;
> -

This still doesn't make sense. If ppos came in as NULL, it should
have become non-NULL long before it reaches this part of the function.

Look at the top of the do_sendfile function, it sets ppos if it is NULL.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt