Re: [PATCH] utimensat() non-conformances and fixes [v3]
From: Miklos Szeredi <hidden>
Date: 2008-05-30 16:37:25
Also in:
linux-fsdevel, lkml
Here's a further version of the patch, against 2.6.26rc2, with the 2008-05-19 git changes you sent me applied. This patch is based on the draft patch you sent me. I've tested this version of the patch, and it works as for all cases except the one mentioned below. But note the following points: 1) I didn't make use of the code in notify_change() that checks IS_IMMUTABLE() and IS_APPEND() (i.e., I did not add ATTR_OWNER_CHECK to the mask in the controlling if statement). Doing this can't easily be made to work for the do_futimes() case without reworking the arguments passed to notify_change(). Actually, I'm inclined to doubt whether it is a good idea to try to roll that check into notify_change() -- at least for utimensat() it seems simpler to not do so.
Ugh... Could we just omit this part (the if !times and write error then check owner)? I know it was my idea, but a) my ideas are often stupid b) one patch should ideally do just one thing After we fixed the original issue, we can still think about this other thing :)
2) I've found yet another divergence from the spec -- but this
was in the original implementation, rather than being
something that has been introduced. In do_futimes() there is
if (!times && !(file->f_mode & FMODE_WRITE))
write_error = -EACCES;
However, the check here should not be against the f_mode (file access
mode), but the against actual permission of the file referred to by
the underlying descriptor. This means that for the do_futimes() +
times==NULL case, a set-user-ID root program could open a file
descriptor O_RDWR/O_WRONLY for which the real UID does not have write
access, and then even after reverting the the effective UID, the real
user could still update file.Sure, but so could a write(2), so that doesn't seem such a big problem. I think we should leave it this way, since changing it would affect not just utimensat() and futimesat() but utime() and utimes() as well, which are well established, old interfaces. Shanging them could in theory break userspace, which we try to avoid if possible. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html