Re: [parch 3/4] vfs: utimensat(): fix error checking for {UTIME_NOW,UTIME_OMIT}case

[Miklos Szeredi]

> 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 <miklos@xxxxxxxxxx>
> CC: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> CC: Ulrich Drepper <drepper@xxxxxxxxxx>
> Signed-off-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
> 
> 
> --- 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);
> 
> 
> 
--
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/