Christoph Hellwig wrote:
> Instead of implementing our own checks use inode_change_ok to check for
> nessecary permission in setattr.
Yeah, the 1st bit I quite like and is similar to what I did in some
nfs4acl code, as you know.
We put all the EPERM cases early on which is nice.
> There is a slight change in behaviour
> as inode_change_ok doesn't allow i_mode updates to add the suid or sgid
> without superuser privilegues while the old XFS code just stripped away
> those bits from the file mode.
>
This bit is of concern for me. And I want to understand.
It seems confusing.
So for xfs, we currently have it that if we try to set the suid/sgid bit
on the mode, and we are not the owner/group-member,
and we don't have CAP_FSETID,
then we clear that bit out of the mode (setuid/setgid) that we were going to set
below.
Now in inode_change_ok() we have some relevant code:
/* Make sure a caller can chmod. */
if (ia_valid & ATTR_MODE) {
if (!is_owner_or_cap(inode))
goto error;
/* Also check the setgid bit! */
if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
inode->i_gid) && !capable(CAP_FSETID))
attr->ia_mode &= ~S_ISGID;
}
It "looks" like it is doing a similar thing for the S_ISGID case but
not for the S_ISUID case.
And then we have similar code in inode_setattr()
if (ia_valid & ATTR_MODE) {
umode_t mode = attr->ia_mode;
if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
mode &= ~S_ISGID;
inode->i_mode = mode;
}
But what about the suid case?
And also, what is with the ATTR_KILL_* bits?
Lemme look...
should_remove_suid:
CAP_FSETID -> return 0
else -> return (S_ISUID ? ATTR_KILL_SUID : 0) | (S_ISGID & S_IXGRP ?
ATTR_KILL_SGID : 0)
do_truncate:
/* Remove suid/sgid on truncate too */
newattrs.ia_valid |= should_remove_suid(dentry);
err = notify_change(dentry, &newattrs);
Oh okay,
so in notify_change() we clear the S_SUID/S_SGID bits in the cases that
ATTR_KILL_SUID/ATTR_KILL_SGID are set and let lower setattr
funcs interpret the KILL bits (as said in a comment).
Hmmm....but it first clears the bits out of the attr->ia_mode
so it does interpret them.
This seems a bit twisty to follow.
--Tim
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c 2008-09-29 18:27:29.000000000
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c 2008-09-29 18:27:32.000000000
> +0200
> @@ -158,56 +161,6 @@ xfs_setattr(
>
> xfs_ilock(ip, lock_flags);
>
> - /* boolean: are we the file owner? */
> - file_owner = (current_fsuid(credp) == ip->i_d.di_uid);
> -
> - /*
> - * Change various properties of a file.
> - * Only the owner or users with CAP_FOWNER
> - * capability may do these things.
> - */
> - if (mask & (ATTR_MODE|ATTR_UID|ATTR_GID)) {
> - /*
> - * CAP_FOWNER overrides the following restrictions:
> - *
> - * The user ID of the calling process must be equal
> - * to the file owner ID, except in cases where the
> - * CAP_FSETID capability is applicable.
> - */
> - if (!file_owner && !capable(CAP_FOWNER)) {
> - code = XFS_ERROR(EPERM);
> - goto error_return;
> - }
> -
> - /*
> - * CAP_FSETID overrides the following restrictions:
> - *
> - * The effective user ID of the calling process shall match
> - * the file owner when setting the set-user-ID and
> - * set-group-ID bits on that file.
> - *
> - * The effective group ID or one of the supplementary group
> - * IDs of the calling process shall match the group owner of
> - * the file when setting the set-group-ID bit on that file
> - */
> - if (mask & ATTR_MODE) {
> - mode_t m = 0;
> -
> - if ((iattr->ia_mode & S_ISUID) && !file_owner)
> - m |= S_ISUID;
> - if ((iattr->ia_mode & S_ISGID) &&
> - !in_group_p((gid_t)ip->i_d.di_gid))
> - m |= S_ISGID;
> -#if 0
> - /* Linux allows this, Irix doesn't. */
> - if ((iattr->ia_mode & S_ISVTX) &&
> !S_ISDIR(ip->i_d.di_mode))
> - m |= S_ISVTX;
> -#endif
> - if (m && !capable(CAP_FSETID))
> - iattr->ia_mode &= ~m;
> - }
> - }
> -
|