The POSIX.1 draft spec for utimensat() says that to do anything
other than setting both timestamps to a time other than the
current time (i.e., times is not NULL, and both tv_nsec fields
are not UTIME_NOW and both tv_nsec fields are not UTIME_OMIT),
either:
a) the caller's effective user ID must match the file owner; or
b) the caller must have appropriate privileges.
If this condition is violated, then the error EPERM should result.
However, the current implementation does not generate EPERM if
one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT.
It should give this error for that case.
This patch:
a) Repairs that problem.
b) Removes the now unneeded nsec_special() helper function.
Miklos suggested an alternative idea, migrating the
is_owner_or_cap() checks 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 gone with this version, which is
simpler, and can be more easily read as being correct.
CC: Miklos Szeredi <[email protected]>
CC: Al Viro <[email protected]>
CC: Ulrich Drepper <[email protected]>
Signed-off-by: Michael Kerrisk <[email protected]>
--- linux-2.6.26-rc4/fs/utimes.c 2008-06-03 23:11:53.000000000 +0200
+++ linux-2.6.26-rc4-utimensat-fix-v4/fs/utimes.c 2008-06-03 23:04:48.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;
@@ -135,8 +130,7 @@
* 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;
@@ -151,6 +145,18 @@
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);
> The POSIX.1 draft spec for utimensat() says that to do anything
> other than setting both timestamps to a time other than the
> current time (i.e., times is not NULL, and both tv_nsec fields
> are not UTIME_NOW and both tv_nsec fields are not UTIME_OMIT),
> either:
>
> a) the caller's effective user ID must match the file owner; or
> b) the caller must have appropriate privileges.
>
> If this condition is violated, then the error EPERM should result.
> However, the current implementation does not generate EPERM if
> one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT.
> It should give this error for that case.
>
> This patch:
>
> a) Repairs that problem.
> b) Removes the now unneeded nsec_special() helper function.
>
> Miklos suggested an alternative idea, migrating the
> is_owner_or_cap() checks 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 gone with this version, which is
> simpler, and can be more easily read as being correct.
Wise decision.
> CC: Miklos Szeredi <[email protected]>
> CC: Al Viro <[email protected]>
> CC: Ulrich Drepper <[email protected]>
> Signed-off-by: Michael Kerrisk <[email protected]>
>
>
> --- linux-2.6.26-rc4/fs/utimes.c 2008-06-03 23:11:53.000000000 +0200
> +++ linux-2.6.26-rc4-utimensat-fix-v4/fs/utimes.c 2008-06-03 23:04:48.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;
> @@ -135,8 +130,7 @@
> * 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) {
This can be restored to be the "else" branch of the "if (times)".
> error = -EACCES;
> if (IS_IMMUTABLE(inode))
> goto mnt_drop_write_and_out;
> @@ -151,6 +145,18 @@
> 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)) {
And I'd rather put this inside the "if (times)" for clarity.
> + error =-EPERM;
> +
> + if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> + goto mnt_drop_write_and_out;
And then you would've realized, that this check was already done.
> +
> + if (!is_owner_or_cap(inode))
> + goto mnt_drop_write_and_out;
OK.
> }
> mutex_lock(&inode->i_mutex);
> error = notify_change(dentry, &newattrs);
>
>
>
> > + } 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)) {
>
> +
> + if (!is_owner_or_cap(inode))
> + goto mnt_drop_write_and_out;
And in fact a little comment wouldn't hurt explaining what exactly is
going on here with the permission checking.
The "/* Don't worry, the checks are done in inode_change_ok() */" is
really reassuring, but unfortunately not exactly the truth (and never
was).
Miklos
> > Miklos suggested an alternative idea, migrating the
> > is_owner_or_cap() checks 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 gone with this version, which is
> > simpler, and can be more easily read as being correct.
>
> Wise decision.
Or maybe not. Isn't this a simpler and more readable patch?
Index: linux/fs/attr.c
===================================================================
--- linux.orig/fs/attr.c 2008-06-03 13:10:11.000000000 +0200
+++ linux/fs/attr.c 2008-06-04 07:07:23.000000000 +0200
@@ -51,7 +51,7 @@ int inode_change_ok(struct inode *inode,
}
/* Check for setting the inode time. */
- if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
+ if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_UPDATE_TIMES)) {
if (!is_owner_or_cap(inode))
goto error;
}
Index: linux/fs/utimes.c
===================================================================
--- linux.orig/fs/utimes.c 2008-06-03 13:10:11.000000000 +0200
+++ linux/fs/utimes.c 2008-06-04 07:10:53.000000000 +0200
@@ -102,9 +102,14 @@ long do_utimes(int dfd, char __user *fil
if (error)
goto dput_and_out;
- /* Don't worry, the checks are done in inode_change_ok() */
newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
if (times) {
+ /*
+ * Checking the owner is done in inode_change_ok(),
+ * even for the special UTIME_OMIT/UTIME_NOW cases.
+ */
+ newattrs.ia_valid |= ATTR_UPDATE_TIMES
+
error = -EPERM;
if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
goto mnt_drop_write_and_out;
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h 2008-06-03 13:10:25.000000000 +0200
+++ linux/include/linux/fs.h 2008-06-04 07:07:43.000000000 +0200
@@ -333,6 +333,7 @@ typedef void (dio_iodone_t)(struct kiocb
#define ATTR_FILE 8192
#define ATTR_KILL_PRIV 16384
#define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
+#define ATTR_UPDATE_TIMES (1 << 16)
/*
* This is the Inode Attributes structure, used for notify_change(). It
On Wed, Jun 4, 2008 at 7:12 AM, Miklos Szeredi <[email protected]> wrote:
>> > Miklos suggested an alternative idea, migrating the
>> > is_owner_or_cap() checks 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 gone with this version, which is
>> > simpler, and can be more easily read as being correct.
>>
>> Wise decision.
>
> Or maybe not. Isn't this a simpler and more readable patch?
The idea is okay (we talked about it already), but I don't like doing
it now because it only migrates *one* of the is_owner_or_cap() checks
out of do_utimes(). We could revisit this later in the context of
migrating both checks out of do_utimes(), but I'd prefer to leave that
for now.
> Index: linux/fs/attr.c
> ===================================================================
> --- linux.orig/fs/attr.c 2008-06-03 13:10:11.000000000 +0200
> +++ linux/fs/attr.c 2008-06-04 07:07:23.000000000 +0200
> @@ -51,7 +51,7 @@ int inode_change_ok(struct inode *inode,
> }
>
> /* Check for setting the inode time. */
> - if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
> + if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_UPDATE_TIMES)) {
> if (!is_owner_or_cap(inode))
> goto error;
> }
> Index: linux/fs/utimes.c
> ===================================================================
> --- linux.orig/fs/utimes.c 2008-06-03 13:10:11.000000000 +0200
> +++ linux/fs/utimes.c 2008-06-04 07:10:53.000000000 +0200
> @@ -102,9 +102,14 @@ long do_utimes(int dfd, char __user *fil
> if (error)
> goto dput_and_out;
>
> - /* Don't worry, the checks are done in inode_change_ok() */
> newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
> if (times) {
> + /*
> + * Checking the owner is done in inode_change_ok(),
> + * even for the special UTIME_OMIT/UTIME_NOW cases.
> + */
> + newattrs.ia_valid |= ATTR_UPDATE_TIMES
> +
> error = -EPERM;
> if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> goto mnt_drop_write_and_out;
> Index: linux/include/linux/fs.h
> ===================================================================
> --- linux.orig/include/linux/fs.h 2008-06-03 13:10:25.000000000 +0200
> +++ linux/include/linux/fs.h 2008-06-04 07:07:43.000000000 +0200
> @@ -333,6 +333,7 @@ typedef void (dio_iodone_t)(struct kiocb
> #define ATTR_FILE 8192
> #define ATTR_KILL_PRIV 16384
> #define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
> +#define ATTR_UPDATE_TIMES (1 << 16)
>
> /*
> * This is the Inode Attributes structure, used for notify_change(). It
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html
On Wed, Jun 4, 2008 at 6:37 AM, Miklos Szeredi <[email protected]> wrote:
>> The POSIX.1 draft spec for utimensat() says that to do anything
>> other than setting both timestamps to a time other than the
>> current time (i.e., times is not NULL, and both tv_nsec fields
>> are not UTIME_NOW and both tv_nsec fields are not UTIME_OMIT),
>> either:
>>
>> a) the caller's effective user ID must match the file owner; or
>> b) the caller must have appropriate privileges.
>>
>> If this condition is violated, then the error EPERM should result.
>> However, the current implementation does not generate EPERM if
>> one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT.
>> It should give this error for that case.
>>
>> This patch:
>>
>> a) Repairs that problem.
>> b) Removes the now unneeded nsec_special() helper function.
>>
>> Miklos suggested an alternative idea, migrating the
>> is_owner_or_cap() checks 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 gone with this version, which is
>> simpler, and can be more easily read as being correct.
>
> Wise decision.
>
>> CC: Miklos Szeredi <[email protected]>
>> CC: Al Viro <[email protected]>
>> CC: Ulrich Drepper <[email protected]>
>> Signed-off-by: Michael Kerrisk <[email protected]>
>>
>>
>> --- linux-2.6.26-rc4/fs/utimes.c 2008-06-03 23:11:53.000000000 +0200
>> +++ linux-2.6.26-rc4-utimensat-fix-v4/fs/utimes.c 2008-06-03 23:04:48.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;
>> @@ -135,8 +130,7 @@
>> * 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) {
>
> This can be restored to be the "else" branch of the "if (times)".
Yes. Thanks.
>> error = -EACCES;
>> if (IS_IMMUTABLE(inode))
>> goto mnt_drop_write_and_out;
>> @@ -151,6 +145,18 @@
>> 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)) {
>
> And I'd rather put this inside the "if (times)" for clarity.
Yes.
>
>> + error =-EPERM;
>> +
>> + if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
>> + goto mnt_drop_write_and_out;
>
> And then you would've realized, that this check was already done.
Quite!
I'll make these changes and test.
Cheers,
Michael
>> +
>> + if (!is_owner_or_cap(inode))
>> + goto mnt_drop_write_and_out;
>
> OK.
>
>> }
>> mutex_lock(&inode->i_mutex);
>> error = notify_change(dentry, &newattrs);
>>
>>
>>
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html