Skip to contentSkip to conversation

lseek/llseek allows the negative offset

DISCUSSION H . J . Lu 2 participants 9 messages Nov 18, 2000 – Nov 20, 2000

Subsystems (2)
lore
Share & tools
lore

H . J . Lu

author
#0 lore raw
# gcc x.c
# ./a.out
lseek on -100000: -100000
write: File too large

Should kernel allow negative offsets for lseek/llseek?

Signature · 66 lines
H.J. Lu ([email protected])
---
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>

extern loff_t llseek (int fd, loff_t offset, int whence);

int
main ()
{
  int fd = open ("/tmp/foo.out", O_CREAT | O_RDWR, 0600);
  off_t res, pos;
  loff_t lres, lpos;
  char buffer [] = "negative offset";

  if (fd < 0)
    {
      perror ("open");
      return 1;
    }

  pos = -100000;
  res = lseek (fd, pos, SEEK_SET);
  if (res == (off_t) -1L)
    {
      perror ("lseek");
      close (fd);
      return 1;
    }
  printf ("lseek on %ld: %ld\n", pos, res);

  if (write (fd, buffer, sizeof (buffer)) != sizeof (buffer))
    {
      perror ("write");
      close (fd);
      return 1;
    }

  lpos = -100000LL;
  lres = llseek (fd, lpos, SEEK_SET);
  if (lres == (loff_t) -1L)
    {
      perror ("llseek");
      close (fd);
      return 1;
    }

  
  printf ("llseek on %lld: %lld\n", lpos, lres);

  if (write (fd, buffer, sizeof (buffer)) != sizeof (buffer))
    {
      perror ("write");
      close (fd);
      return 1;
    }

  close (fd);

  return 0;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
author
#1 lore raw
Quoted 8 lines · H . J . Lu
# gcc x.c
# ./a.out
lseek on -100000: -100000
write: File too large

Should kernel allow negative offsets for lseek/llseek?

Never mind. I was running the wrong kernel.

H.J.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw
In reply to H . J . Lu
Quoted 11 lines · H . J . Lu
Quoted 8 lines · H . J . Lu
# gcc x.c
# ./a.out
lseek on -100000: -100000
write: File too large

Should kernel allow negative offsets for lseek/llseek?

Never mind. I was running the wrong kernel.

With 2.2.18pre21aa2 this little proggy:

main()

{
	int fd = creat("x", 0600);
	lseek(fd, 0x80000000, 0);
}

get confused this way:

lseek(3, 2147483648, SEEK_SET) = -1 ERRNO_0 (Success)
_exit(-2147483648) = ?

I fixed it this way:

fs/read_write.c +12 -2
diff -urN 2.2.18pre21/fs/read_write.c lseek/fs/read_write.c
@@ -53,6 +53,10 @@
 	struct dentry * dentry;
 	struct inode * inode;
 
+	retval = -EINVAL;
+	if (offset < 0)
+		goto out_nolock;
+
 	lock_kernel();
 	retval = -EBADF;
 	file = fget(fd);
@@ -69,6 +73,7 @@
 	fput(file);
 bad:
 	unlock_kernel();
+out_nolock:
 	return retval;
 }
 
@@ -83,6 +88,11 @@
 	struct inode * inode;
 	loff_t offset;
 
+	retval = -EINVAL;
+	offset = ((loff_t) offset_high << 32) | offset_low;
+	if (offset < 0)
+		goto out_nolock;
+
 	lock_kernel();
 	retval = -EBADF;
 	file = fget(fd);
@@ -96,8 +106,7 @@
 	if (origin > 2)
 		goto out_putf;
 
-	offset = llseek(file, ((loff_t) offset_high << 32) | offset_low,
-			origin);
+	offset = llseek(file, offset, origin);
 
 	retval = (int)offset;
 	if (offset >= 0) {
@@ -109,6 +118,7 @@
 	fput(file);
 bad:
 	unlock_kernel();
+out_nolock:
 	return retval;
 }
 #endif

I've not tried yet in practice but by reading sources 2.4.x has the same bug since doing the check internally to ext2 is too late to handle the 32bit (non-lfs) lseek interface. After moving the checks into the vfs (that seems the right thing to do to me) the check internally to ext2 can be removed of course (it was superflous anyways because ext2 file size is limited to 4T that's not negative value in 64bit signed math)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

author
#3 lore raw
In reply to H . J . Lu
Quoted 32 lines · Andrea Arcangeli
Quoted 11 lines · H . J . Lu
Quoted 8 lines · H . J . Lu
# gcc x.c
# ./a.out
lseek on -100000: -100000
write: File too large

Should kernel allow negative offsets for lseek/llseek?

Never mind. I was running the wrong kernel.

With 2.2.18pre21aa2 this little proggy:

main()

{
	int fd = creat("x", 0600);
	lseek(fd, 0x80000000, 0);
}

get confused this way:

lseek(3, 2147483648, SEEK_SET) = -1 ERRNO_0 (Success)
_exit(-2147483648) = ?

I fixed it this way:

diff -urN 2.2.18pre21/fs/read_write.c lseek/fs/read_write.c

fs/read_write.c line 59

Quoted 7 lines
fs/read_write.c +4 -0
@@ -53,6 +53,10 @@
 	struct dentry * dentry;
 	struct inode * inode;
 
+	retval = -EINVAL;
+	if (offset < 0)
+		goto out_nolock;
+

offset shouldn't be < 0 to begin with. There may be a bug somewhere else. In my case,

	if (offset < 0)
		return -EINVAL;

is missing from those FS lseek functions.

H.J.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

author
#5 lore raw
In reply to H . J . Lu
Quoted 6 lines · Andrea Arcangeli
Quoted 1 line · Andrea Arcangeli

I fixed it this way:

fix is plain wrong, it's still possible to have lseek return -1 -2 -3 -4 even when it should return -EINVAL.

Try this again 2.2.18pre21. It works for me.

Signature · 41 lines
H.J. Lu ([email protected])
---
--- linux/fs/ext2/file.c.lseek	Sat Nov 18 17:18:49 2000
+++ linux/fs/ext2/file.c	Sat Nov 18 17:19:28 2000
@@ -120,6 +120,8 @@ static long long ext2_file_lseek(
 		case 1:
 			offset += file->f_pos;
 	}
+	if (offset < 0)
+		return -EINVAL;
 	if (((unsigned long long) offset >> 32) != 0) {
 #if BITS_PER_LONG < 64
 		return -EINVAL;
--- linux/fs/proc/mem.c.lseek	Tue Jan  4 10:12:23 2000
+++ linux/fs/proc/mem.c	Sat Nov 18 17:19:28 2000
@@ -196,14 +196,17 @@ static long long mem_lseek(struct file *
 {
 	switch (orig) {
 		case 0:
-			file->f_pos = offset;
-			return file->f_pos;
+			break;
 		case 1:
-			file->f_pos += offset;
-			return file->f_pos;
+			offset += file->f_pos;
+			break;
 		default:
 			return -EINVAL;
 	}
+	if (offset < 0)
+			return -EINVAL;
+	file->f_pos = offset;
+	return offset;
 }
 
 /*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#6 lore raw
In reply to H . J . Lu
Quoted 9 lines · H . J . Lu

Try this again 2.2.18pre21. It works for me.

Signature · 2 lines
H.J. Lu ([email protected])
---

fs/ext2/file.c static long long ext2_file_lseek( · line 127

Quoted 8 lines
fs/ext2/file.c renamed from fs/ext2/file.c.lseek +2 -0
@@ -120,6 +120,8 @@ static long long ext2_file_lseek(
 		case 1:
 			offset += file->f_pos;
 	}
+	if (offset < 0)
+		return -EINVAL;
 	if (((unsigned long long) offset >> 32) != 0) {
 #if BITS_PER_LONG < 64
 		return -EINVAL;

It's not enough for 2.2.x (and you left the `>> 32' nosense check).

2.2.x vanilla (so non-lfs) is getting wrong both 32bit and 64bit:

1) 32bit can lseek over 2G and it can return bogus retval

main()

{
        int fd = creat("x", 0600);
        lseek(fd, 0x7fffffff, 1);
        lseek(fd, 0x7fffffff, 1);
}

lseek(3, 2147483647, SEEK_CUR) = 2147483647
lseek(3, 2147483647, SEEK_CUR) = -1 ENOENT (No such file or directory)

(this isn't really -ENOENT but it's just trying to return a succesful -2 value)

2) 64bit can lseek over ext2_max_sizes and it can return bogus retvals

main()

{
        int fd = creat("x", 0600);
        lseek(fd, -2, 0);
}

open("x", O_WRONLY|O_CREAT|O_TRUNC, 0600) = 3
lseek(3, 18446744073709551614, SEEK_SET) = -1 ENOENT (No such file or directory)

(this isn't really -ENOENT but it's just trying to return a succesful -2 value)

This will cure 2.2.x vanilla:

fs/ext2/file.c renamed from fs/ext2/file.c.~1~ +4 -4
@@ -120,14 +120,14 @@
 		case 1:
 			offset += file->f_pos;
 	}
-	if (((unsigned long long) offset >> 32) != 0) {
 #if BITS_PER_LONG < 64
+	if (offset >> 31)
 		return -EINVAL;
 #else
-		if (offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
-			return -EINVAL;
+	if (offset < 0 ||
+	    offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
+		return -EINVAL;
 #endif
-	} 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
 		file->f_reada = 0;

and this will remove the nosense stuff from 2.4.x:

fs/ext2/file.c renamed from fs/ext2/file.c.~1~ +2 -5
@@ -53,12 +53,9 @@
 		case 1:
 			offset += file->f_pos;
 	}
-	if (offset<0)
+	if (offset < 0 ||
+	    offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
 		return -EINVAL;
-	if (((unsigned long long) offset >> 32) != 0) {
-		if (offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
-			return -EINVAL;
-	} 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
 		file->f_reada = 0;

(the above is also ok for 2.2.x-lfs and I'll fix it that way)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

author
#7 lore raw
In reply to H . J . Lu
Quoted 4 lines · Andrea Arcangeli
Quoted 3 lines · H . J . Lu

Try this again 2.2.18pre21. It works for me.

Quoted 17 lines
Quoted 6 lines
Signature · 2 lines
H.J. Lu ([email protected])
---

fs/ext2/file.c static long long ext2_file_lseek( · line 127

Quoted 8 lines
fs/ext2/file.c renamed from fs/ext2/file.c.lseek +2 -0
@@ -120,6 +120,8 @@ static long long ext2_file_lseek(
 		case 1:
 			offset += file->f_pos;
 	}
+	if (offset < 0)
+		return -EINVAL;
 	if (((unsigned long long) offset >> 32) != 0) {
 #if BITS_PER_LONG < 64
 		return -EINVAL;

It's not enough for 2.2.x (and you left the `>> 32' nosense check).

It works for me on ia32. offset is a long long in 2.2.18pre21 on ia32. I don't see anything wrong for

	if (offset < 0)
		return -EINVAL;

I got:

lseek(3, 4294967295, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967294, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967293, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967292, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967291, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 2147483647, SEEK_CUR)          = -1 EOVERFLOW (Value too large for defined data type)
lseek(3, 2147483647, SEEK_CUR)          = -1 EOVERFLOW (Value too large for defined data type)

_llseek(3, 18446744073709551614, 0xbffff980, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294867296, SEEK_SET) = -1 EINVAL (Invalid argument)

My kernel has LFS patch.

H.J.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#8 lore raw
In reply to H . J . Lu
Quoted 12 lines · H . J . Lu
fs/proc/mem.c renamed from fs/proc/mem.c.lseek +1 -4
@@ -196,14 +196,17 @@ static long long mem_lseek(struct file *
 {
 	switch (orig) {
 		case 0:
-			file->f_pos = offset;
-			return file->f_pos;
+			break;
 		case 1:
-			file->f_pos += offset;
-			return file->f_pos;

fs/proc/mem.c static long long mem_lseek(struct file * · line 210

Quoted 10 lines
fs/proc/mem.c renamed from fs/proc/mem.c.lseek +6 -0
+			offset += file->f_pos;
+			break;
 		default:
 			return -EINVAL;
 	}
+	if (offset < 0)
+			return -EINVAL;
+	file->f_pos = offset;
+	return offset;
 }

This looks fine to me.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/