utimensat() non-conformances and fixes [v4] (patch)

[Michael Kerrisk <mtk.manpages@googlemail.com>]

Andrew,

This patch fixes all of the utimensat() bugs outlined in my
previous mail.

I could have split the patch out into a few pieces, but given
that it is not long, and all against a single file, I've made
one patch.  Let me know if you would like separate patches for
the pieces below.

Miklos suggested an alternative idea, migrating the
is_owner_or_cap() check into fs/attr.c:inode_change_ok() via
the use of an ATTR_OWNER_CHECK flag.  Maybe we could do that
later, but for now I've one with this version, which is
simpler, and can be more easily read as being correct.

Roughly, the changes accomplish the following:

==
@@ -102,11 +97,15 @@

Addresses bug 4, and simplifies the code, since the
times == NULL and times == {UTIME_NOW, UTIME_NOW} should,
according to the POSIX.1 draft, be exactly equivalent.

==
@@ -131,15 +130,16 @@
[...]
 		if (!is_owner_or_cap(inode)) {
 			if (f) {
-				if (!(f->f_mode & FMODE_WRITE))
+				error = permission(inode, MAY_WRITE, NULL);
+				if (error)

Addresses bug 2.

==
@@ -169,14 +182,6 @@

Addresses bug 3.

==
@@ -147,7 +147,20 @@

Addresses bug 1

==

The patch also

a) removes the now unneeded nsec_special() helper function.

b) Makes a whitespace cleanup (tabs instead of spaces).

==

Thanks to Miklos, who's comments helped me improve and complete
the patch.

Cheers,

Michael


Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

--- linux-2.6.26-rc4/fs/utimes.c	2008-05-27 14:24:13.000000000 +0200
+++ linux-2.6.26-rc4-utimensat-fix-v4/fs/utimes.c	2008-06-03 15:38:53.000000000 +0200
@@ -40,14 +40,9 @@

 #endif

-static bool nsec_special(long nsec)
-{
-	return nsec == UTIME_OMIT || nsec == UTIME_NOW;
-}
-
 static bool nsec_valid(long nsec)
 {
-	if (nsec_special(nsec))
+	if (nsec == UTIME_OMIT || nsec == UTIME_NOW)
 		return true;

 	return nsec >= 0 && nsec <= 999999999;
@@ -102,11 +97,15 @@
 	if (error)
 		goto dput_and_out;

+	if (times && times[0].tv_nsec == UTIME_NOW &&
+		     times[1].tv_nsec == UTIME_NOW)
+		times = NULL;
+
 	/* Don't worry, the checks are done in inode_change_ok() */
 	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
 	if (times) {
 		error = -EPERM;
-                if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+		if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
 			goto mnt_drop_write_and_out;

 		if (times[0].tv_nsec == UTIME_OMIT)
@@ -131,15 +130,16 @@
 	 * UTIME_NOW, then need to check permissions, because
 	 * inode_change_ok() won't do it.
 	 */
-	if (!times || (nsec_special(times[0].tv_nsec) &&
-		       nsec_special(times[1].tv_nsec))) {
+	if (!times) {
 		error = -EACCES;
+
                 if (IS_IMMUTABLE(inode))
 			goto mnt_drop_write_and_out;

 		if (!is_owner_or_cap(inode)) {
 			if (f) {
-				if (!(f->f_mode & FMODE_WRITE))
+				error = permission(inode, MAY_WRITE, NULL);
+				if (error)
 					goto mnt_drop_write_and_out;
 			} else {
 				error = vfs_permission(&nd, MAY_WRITE);
@@ -147,7 +147,20 @@
 					goto mnt_drop_write_and_out;
 			}
 		}
+	} else if ((times[0].tv_nsec == UTIME_NOW &&
+		    times[1].tv_nsec == UTIME_OMIT)
+		  ||
+		   (times[0].tv_nsec == UTIME_OMIT &&
+		    times[1].tv_nsec == UTIME_NOW)) {
+		error =-EPERM;
+
+		if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+			goto mnt_drop_write_and_out;
+
+		if (!is_owner_or_cap(inode))
+			goto mnt_drop_write_and_out;
 	}
+
 	mutex_lock(&inode->i_mutex);
 	error = notify_change(dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
@@ -169,14 +182,6 @@
 	if (utimes) {
 		if (copy_from_user(&tstimes, utimes, sizeof(tstimes)))
 			return -EFAULT;
-		if ((tstimes[0].tv_nsec == UTIME_OMIT ||
-		     tstimes[0].tv_nsec == UTIME_NOW) &&
-		    tstimes[0].tv_sec != 0)
-			return -EINVAL;
-		if ((tstimes[1].tv_nsec == UTIME_OMIT ||
-		     tstimes[1].tv_nsec == UTIME_NOW) &&
-		    tstimes[1].tv_sec != 0)
-			return -EINVAL;

 		/* Nothing to do, we must not even check the path.  */
 		if (tstimes[0].tv_nsec == UTIME_OMIT &&