2014-01-29 11:57:28

by Nick Alcock

Subject: [PATCH] vfs: respect FMODE_UNSIGNED_OFFSET in p(read|write)[v]*().

Because the pread and pwrite functions do not respect the unsigned
offset flag, you can read certain parts of /proc/$pid/mem via lseek()
and read(), but not via pread(). (This probably went unnoticed
because on i386 and x86-64, almost everything except the vdso is
normally located below the region where signed offsets become
negative: but this is not true on all platforms.)

Fixing pwrite() is currently academic because this flag is only
used by files that do not allow writing, but it is easiest to
be consistent and retain a similarity of form between the pread*()
and pwrite*() functions.

Signed-off-by: Nick Alcock <[email protected]>
---
fs/read_write.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 58e440d..f33f664 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -534,10 +534,12 @@ SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf,
struct fd f;
ssize_t ret = -EBADF;

- if (pos < 0)
+ f = fdget(fd);
+ if ((pos < 0) && (!f.file || !unsigned_offsets(f.file))) {
+ fdput(f);
return -EINVAL;
+ }

- f = fdget(fd);
if (f.file) {
ret = -ESPIPE;
if (f.file->f_mode & FMODE_PREAD)
@@ -554,10 +556,12 @@ SYSCALL_DEFINE4(pwrite64, unsigned int, fd, const char __user *, buf,
struct fd f;
ssize_t ret = -EBADF;

- if (pos < 0)
+ f = fdget(fd);
+ if ((pos < 0) && (!f.file || !unsigned_offsets(f.file))) {
+ fdput(f);
return -EINVAL;
+ }

- f = fdget(fd);
if (f.file) {
ret = -ESPIPE;
if (f.file->f_mode & FMODE_PWRITE)
@@ -847,10 +851,12 @@ SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
struct fd f;
ssize_t ret = -EBADF;

- if (pos < 0)
+ f = fdget(fd);
+ if ((pos < 0) && (!f.file || !unsigned_offsets(f.file))) {
+ fdput(f);
return -EINVAL;
+ }

- f = fdget(fd);
if (f.file) {
ret = -ESPIPE;
if (f.file->f_mode & FMODE_PREAD)
@@ -871,8 +877,10 @@ SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
struct fd f;
ssize_t ret = -EBADF;

- if (pos < 0)
+ if ((pos < 0) && (!f.file || !unsigned_offsets(f.file))) {
+ f = fdget(fd);
return -EINVAL;
+ }

f = fdget(fd);
if (f.file) {
--
1.8.5.2.169.ge058798

2014-01-29 15:18:16

by Al Viro

Subject: Re: [PATCH] vfs: respect FMODE_UNSIGNED_OFFSET in p(read|write)[v]*().

On Wed, Jan 29, 2014 at 11:57:20AM +0000, Nick Alcock wrote:
> ssize_t ret = -EBADF;
>
> - if (pos < 0)
> + f = fdget(fd);
> + if ((pos < 0) && (!f.file || !unsigned_offsets(f.file))) {
> + fdput(f);
> return -EINVAL;
> + }

... and now pread(-1, ...) fails with EINVAL instead of EBADF.

2014-01-29 16:53:29

by Nick Alcock

Subject: Re: [PATCH] vfs: respect FMODE_UNSIGNED_OFFSET in p(read|write)[v]*().

On 29 Jan 2014, Al Viro outgrape:

> On Wed, Jan 29, 2014 at 11:57:20AM +0000, Nick Alcock wrote:
>> ssize_t ret = -EBADF;
>>
>> - if (pos < 0)
>> + f = fdget(fd);
>> + if ((pos < 0) && (!f.file || !unsigned_offsets(f.file))) {
>> + fdput(f);
>> return -EINVAL;
>> + }
>
> ... and now pread(-1, ...) fails with EINVAL instead of EBADF.

Sorry, I don't see it. If the fh is invalid, control flow is unchanged
unless pos is also < 0 (that's an && outside the bracketed section, not
an ||, and nothing I've touched changes ret outside that conditional
branch): if pos *is* < 0, we'd have had an EINVAL before and we have one
now, likewise unchanged.

What am I missing?

(Or did you miss the brackets enclosing (!f.file || !unsigned_offsets(f.file))?
If so, I'm not surprised: it would really be easier to read if that
function had the inverse sense, 'signed_offsets()'...)

--
NULL && (void)