32 bit Linux lseek missing overflow check (was: Re: Linuxolator patches: stat and lseek SEEK_END for disk devices)

[John Baldwin]

On Monday 01 March 2010 1:25:53 pm John Baldwin wrote:
> On Friday 26 February 2010 4:09:52 pm Juergen Lock wrote:
> > On Fri, Feb 26, 2010 at 11:21:05AM -0500, John Baldwin wrote:
> > > On Thursday 25 February 2010 3:28:50 pm Juergen Lock wrote:
> > > > On Tue, Feb 23, 2010 at 10:50:10PM +0100, Juergen Lock wrote:
> > > > > Hi!
> > > > > 
> > > > >  Before this gets buried on -hackers in another thead... :)
> > > > > 
> > > > >  I now have disks appear as block devices for Linux processes (there
> > > > > already was commented out code for that in linux_stats.c, I hope my
> > > > > version is now `correct enough' to be usable [1]), and I made a simple
> > > > > patch to make lseek SEEK_END (L_XTND in the source) dtrt on disk
> > > > > devices too by simply invoking the DIOCGMEDIASIZE ioctl there; [2]
> > > > > both of these things are what (some) Linux processes expect.
> > > > > 
> > > > >  Patches are here: (made on stable/8, if they don't apply on head
> > > > > I'll have to make extra versions for that...)
> > > > > 	http://people.freebsd.org/~nox/linuxdisk-blk.patch [1]
> > > > > 	http://people.freebsd.org/~nox/lseek-seek_end.patch [2]
> > > > > 
> > > > >  And yes, with these patches the Linux bsdtar mentioned on -hackers
> > > > > in the `"tar tfv /dev/cd0" speedup patch' thread now also runs fast
> > > > > on FreeBSD. :)
> > > > 
> > > > I now added an vn_isdisk() check to the second patch after comments from
> > > > julian, and I made a new patch that adds an overflow check to the 32 bit
> > > > linux lseek: (also at
> > > > 	http://people.freebsd.org/~nox/linux-lseek-overflow.patch
> > > > )
>
> Unfortunately it panics in g_dev_ioctl() with INVARIANTS enabled.  I will bug 
> phk about that.

After talking to phk, it isn't safe to invoke the ioctl from stat(), only if
the device is known to be opened (e.g. lseek() or fstat()).  This patch is a
simpler version of your original patch (with some simple error checking
added).  I wasn't able to test it due to weird issues with igb(4) on my test
machine, but it does compile:

Index: vfs_syscalls.c
===================================================================
--- vfs_syscalls.c	(revision 204518)
+++ vfs_syscalls.c	(working copy)
@@ -45,6 +45,7 @@
 #include <sys/systm.h>
 #include <sys/bio.h>
 #include <sys/buf.h>
+#include <sys/disk.h>
 #include <sys/sysent.h>
 #include <sys/malloc.h>
 #include <sys/mount.h>
@@ -1920,7 +1921,7 @@
 	struct file *fp;
 	struct vnode *vp;
 	struct vattr vattr;
-	off_t offset;
+	off_t offset, size;
 	int error, noneg;
 	int vfslocked;
 
@@ -1951,6 +1952,15 @@
 		VOP_UNLOCK(vp, 0);
 		if (error)
 			break;
+
+		/*
+		 * If the file references a disk device, then fetch
+		 * the media size and use that to determine the ending
+		 * offset.
+		 */
+		if (vattr.va_size == 0 && vp->v_type == VCHR &&
+		    fo_ioctl(fp, DIOCGMEDIASIZE, &size, cred, td) == 0)
+			vattr.va_size = size;
 		if (noneg &&
 		    (vattr.va_size > OFF_MAX ||
 		    (offset > 0 && vattr.va_size > OFF_MAX - offset))) {

-- 
John Baldwin