Skip to contentSkip to conversation

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

PATCH In review Nick Alcock 2 participants 3 messages Jan 29, 2014 VFS

lore
Share & tools
lore

Nick Alcock

author
#0 lore raw

PATCH 1/1 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(-)
fs/read_write.c +15 -7
index 58e440d..f33f664 100644
@@ -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) {
Signature · 1 line
1.8.5.2.169.ge058798
maintainer
#1 lore raw

fs/read_write.c SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf, · line 541

Quoted 8 lines · Nick Alcock
fs/read_write.c +4 -1
 	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.

author
#2 lore raw

On 29 Jan 2014, Al Viro outgrape:

Quoted 11 lines · Nick Alcock

fs/read_write.c SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf, · line 541

Quoted 8 lines
fs/read_write.c +4 -1
 	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()'...)

Signature · 1 line
NULL && (void)