Re: [PATCH] [resend] VFS locking errors on max offset edge cases

[Bruce Allan]

Resending patch which also applies to 2.6.11-rc4.

On Thu, 2004-12-23 at 19:26, Matthew Wilcox wrote: 
> On Thu, Dec 23, 2004 at 03:01:20PM -0800, Bruce Allan wrote:
> > A number of Connectathon (http://www.connectathon.org/nfstests.html)
> > POSIX/fcntl() locking tests fail (even on local filesystems) at various
> > edge cases (i.e. around maximum allowable offsets) on 64-bit
> > architectures.
> 
> OK, that's bad and needs to be fixed.
> 
> > The overflow tests in fs/compat.c were superfluous where they were
> > located because if there was a conflicting lock, l_start and l_len would
> > have been overwritten with the values owned by the conflicting lock; if
> > no conflicting lock, sys_fcntl() would have returned any applicable
> > error.  The tests are moved above the call to sys_fcntl() to capture
> > overflow errors which would not have been caught by sys_fcntl(), eg.
> > obvious overflow when _FILE_OFFSET_BITS == 32.
> 
> I don't buy this explanation though.  With your patch, we're testing
> the lock the user passed in to see if it'd overflow.  Clearly, that
> can never happen.  The checks are supposed to be testing whether the
> conflicting lock is outside the limits that a program using a 32-bit
> off_t can cope with.

Hi Matthew,

I now agree with you not buying my initial explanation, but have some
follow-up comments/questions.  How is it clear that a user can never
pass in a lock request that would overflow?  AFAICT, there is no reason
a 32-bit application could not pass down a request for a lock (eg.
l_whence=SEEK_SET, l_start=0x7fffffff, l_len=2) which should overflow.

For 64-bit applications on a 64-bit architecture (and 32-bit apps/32-bit
archs) any overflow error in a lock request would be caught in
flock_to_posix_lock() whether or not there is a conflicting lock.

For 32-bit applications with 32-bit userland off_t on a 64-bit
architecture, flock_to_posix_lock() does not capture overflow errors on
requested locks because that function is checking for overflow assuming
it is a 64-bit value.  If there is no conflicting lock these values are
passed back to compat_sys_fcntl64() and the overflow check in that
function should catch it (provided l_whence==SEEK_SET, otherwise that
check may not catch it), but if there is a conflicting lock the overflow
check in compat_sys_fcntl64() is checking the values on the conflicting
lock instead of the requested lock.

I do see your point now about compat_sys_fcntl64() "checks are supposed
to be testing whether the conflicting lock is outside the limits that a
program using a 32-bit off_t can cope with", but it seems to me there
still needs to be an overflow check of the requested lock in
compat_sys_fcntl64() for the case of F_GETLK/F_SETLK/F_SETLKW; something
akin to the check in flock_to_posix_lock().  It's duplicate code, I
know, but at the moment I'm at a loss for a better alternative (I
suppose it could be setup instead to pass the maximum filesize all the
way to flock_to_posix_lock()).  This check would be unnecessary for
F_GETLK64/F_SETLK64/F_SETLKW64 as it would be properly handled in
flock_to_posix_lock().

Comments?

> > These tests also had a couple 'off by one' errors when comparing with
> > the maximum allowable offset.
> 
> Perhaps just fix that, and don't move the tests around?
[snip] 
> I hate assignments inside if statements.  Make this:
> 
> 	start += l->l_start;
> 	if (start < 0)
> 		return -EINVAL;
Done.

Signed-off-by: Bruce Allan <bwa@xxxxxxxxxx>

diff -Nurp -Xdontdiff linux-2.6.10-rc3-vanilla/fs/compat.c linux-2.6.10-rc3/fs/compat.c
--- linux-2.6.10-rc3-vanilla/fs/compat.c	2004-12-23 11:52:50.000000000 -0800
+++ linux-2.6.10-rc3/fs/compat.c	2004-12-30 13:33:52.525317789 -0800
@@ -523,6 +523,40 @@ static int put_compat_flock64(struct flo
 }
 #endif
 
+static int check_compat_flock(unsigned int fd, struct flock *l)
+{
+	compat_off_t start, end;
+	struct file *filp = fget(fd);
+
+	switch (l->l_whence) {
+	case 0: /*SEEK_SET*/
+		start = 0;
+		break;
+	case 1: /*SEEK_CUR*/
+		start = filp->f_pos;
+		break;
+	case 2: /*SEEK_END*/
+		start = i_size_read(filp->f_dentry->d_inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* POSIX-1996 leaves the case l->l_len < 0 undefined;
+	   POSIX-2001 defines it. */
+	start += l->l_start;
+	end = start + l->l_len - 1;
+	if (l->l_len < 0) {
+		end = start - 1;
+		start += l->l_len;
+	}
+	if (start < 0)
+		return -EINVAL;
+	if (l->l_len > 0 && end < 0)
+		return -EOVERFLOW;
+	return 0;
+}
+
 asmlinkage long compat_sys_fcntl64(unsigned int fd, unsigned int cmd,
 		unsigned long arg)
 {
@@ -537,13 +571,18 @@ asmlinkage long compat_sys_fcntl64(unsig
 		ret = get_compat_flock(&f, compat_ptr(arg));
 		if (ret != 0)
 			break;
+		ret = check_compat_flock(fd, &f);
+		if (ret != 0)
+			break;
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
 		ret = sys_fcntl(fd, cmd, (unsigned long)&f);
 		set_fs(old_fs);
 		if (cmd == F_GETLK && ret == 0) {
-			if ((f.l_start >= COMPAT_OFF_T_MAX) ||
-			    ((f.l_start + f.l_len) > COMPAT_OFF_T_MAX))
+			/* check for overflow of conflicting lock if any */
+			if ((f.l_whence != F_UNLCK) &&
+			    ((f.l_start > COMPAT_OFF_T_MAX) ||
+			     ((f.l_start + f.l_len - 1) > COMPAT_OFF_T_MAX)))
 				ret = -EOVERFLOW;
 			if (ret == 0)
 				ret = put_compat_flock(&f, compat_ptr(arg));
@@ -563,8 +602,10 @@ asmlinkage long compat_sys_fcntl64(unsig
 				(unsigned long)&f);
 		set_fs(old_fs);
 		if (cmd == F_GETLK64 && ret == 0) {
-			if ((f.l_start >= COMPAT_LOFF_T_MAX) ||
-			    ((f.l_start + f.l_len) > COMPAT_LOFF_T_MAX))
+			/* check for overflow of conflicting lock if any */
+			if ((f.l_whence != F_UNLCK) && 
+			    ((f.l_start > COMPAT_LOFF_T_MAX) ||
+			     ((f.l_start + f.l_len - 1) > COMPAT_LOFF_T_MAX)))
 				ret = -EOVERFLOW;
 			if (ret == 0)
 				ret = put_compat_flock64(&f, compat_ptr(arg));
diff -Nurp -Xdontdiff linux-2.6.10-rc3-vanilla/fs/locks.c linux-2.6.10-rc3/fs/locks.c
--- linux-2.6.10-rc3-vanilla/fs/locks.c	2004-12-23 11:52:50.000000000 -0800
+++ linux-2.6.10-rc3/fs/locks.c	2004-12-30 13:33:52.529317402 -0800
@@ -315,6 +315,8 @@ static int flock_to_posix_lock(struct fi
 	/* POSIX-1996 leaves the case l->l_len < 0 undefined;
 	   POSIX-2001 defines it. */
 	start += l->l_start;
+	if (start < 0)
+		return -EINVAL;
 	end = start + l->l_len - 1;
 	if (l->l_len < 0) {
 		end = start - 1;


---
Bruce Allan  <bwa@xxxxxxxxxx>
Software Engineer, Linux Technology Center
IBM Corporation, Beaverton OR USA

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/