[PATCH 3/3] use inode_change_ok for setattr permission checking

[PATCH 3/3] use inode_change_ok for setattr permission checking

[Christoph Hellwig]

Instead of implementing our own checks use inode_change_ok to check for
nessecary permission in setattr.  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.


Signed-off-by: Christoph Hellwig <hch@lst.de>

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
@@ -70,7 +70,6 @@ xfs_setattr(
 	gid_t			gid=0, igid=0;
 	int			timeflags = 0;
 	struct xfs_dquot	*udqp, *gdqp, *olddquot1, *olddquot2;
-	int			file_owner;
 	int			need_iolock = 1;
 
 	xfs_itrace_entry(ip);
@@ -81,6 +80,10 @@ xfs_setattr(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	code = -inode_change_ok(inode, iattr);
+	if (code)
+		return code;
+
 	olddquot1 = olddquot2 = NULL;
 	udqp = gdqp = NULL;
 
@@ -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;
-		}
-	}
-
 	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
@@ -224,22 +177,6 @@ xfs_setattr(
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
 		/*
-		 * CAP_CHOWN overrides the following restrictions:
-		 *
-		 * If _POSIX_CHOWN_RESTRICTED is defined, this capability
-		 * shall override the restriction that a process cannot
-		 * change the user ID of a file it owns and the restriction
-		 * that the group ID supplied to the chown() function
-		 * shall be equal to either the group ID or one of the
-		 * supplementary group IDs of the calling process.
-		 */
-		if ((iuid != uid ||
-		     (igid != gid && !in_group_p((gid_t)gid))) &&
-		    !capable(CAP_CHOWN)) {
-			code = XFS_ERROR(EPERM);
-			goto error_return;
-		}
-		/*
 		 * Do a quota reservation only if uid/gid is actually
 		 * going to change.
 		 */
@@ -284,19 +221,6 @@ xfs_setattr(
 	}
 
 	/*
-	 * Change file access or modified times.
-	 */
-	if (mask & (ATTR_ATIME|ATTR_MTIME)) {
-		if (!file_owner) {
-			if ((mask & (ATTR_MTIME_SET|ATTR_ATIME_SET)) &&
-			    !capable(CAP_FOWNER)) {
-				code = XFS_ERROR(EPERM);
-				goto error_return;
-			}
-		}
-	}
-
-	/*
 	 * Now we can make the changes.  Before we join the inode
 	 * to the transaction, if ATTR_SIZE is set then take care of
 	 * the part of the truncation that must be done without the

Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Christoph Hellwig]

On Mon, Sep 29, 2008 at 11:53:29PM +0200, Christoph Hellwig wrote:
> Instead of implementing our own checks use inode_change_ok to check for
> nessecary permission in setattr.  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 one needs a slight respin for the 2.6.27-rc8 merge:


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-10-03 22:16:03.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-10-03 22:16:32.000000000 +0200
@@ -70,7 +70,6 @@ xfs_setattr(
 	gid_t			gid=0, igid=0;
 	int			timeflags = 0;
 	struct xfs_dquot	*udqp, *gdqp, *olddquot1, *olddquot2;
-	int			file_owner;
 	int			need_iolock = 1;
 
 	xfs_itrace_entry(ip);
@@ -81,6 +80,10 @@ xfs_setattr(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	code = -inode_change_ok(inode, iattr);
+	if (code)
+		return code;
+
 	olddquot1 = olddquot2 = NULL;
 	udqp = gdqp = NULL;
 
@@ -158,56 +161,6 @@ xfs_setattr(
 
 	xfs_ilock(ip, lock_flags);
 
-	/* boolean: are we the file owner? */
-	file_owner = (current_fsuid() == 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;
-		}
-	}
-
 	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
@@ -224,22 +177,6 @@ xfs_setattr(
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
 		/*
-		 * CAP_CHOWN overrides the following restrictions:
-		 *
-		 * If _POSIX_CHOWN_RESTRICTED is defined, this capability
-		 * shall override the restriction that a process cannot
-		 * change the user ID of a file it owns and the restriction
-		 * that the group ID supplied to the chown() function
-		 * shall be equal to either the group ID or one of the
-		 * supplementary group IDs of the calling process.
-		 */
-		if ((iuid != uid ||
-		     (igid != gid && !in_group_p((gid_t)gid))) &&
-		    !capable(CAP_CHOWN)) {
-			code = XFS_ERROR(EPERM);
-			goto error_return;
-		}
-		/*
 		 * Do a quota reservation only if uid/gid is actually
 		 * going to change.
 		 */
@@ -284,19 +221,6 @@ xfs_setattr(
 	}
 
 	/*
-	 * Change file access or modified times.
-	 */
-	if (mask & (ATTR_ATIME|ATTR_MTIME)) {
-		if (!file_owner) {
-			if ((mask & (ATTR_MTIME_SET|ATTR_ATIME_SET)) &&
-			    !capable(CAP_FOWNER)) {
-				code = XFS_ERROR(EPERM);
-				goto error_return;
-			}
-		}
-	}
-
-	/*
 	 * Now we can make the changes.  Before we join the inode
 	 * to the transaction, if ATTR_SIZE is set then take care of
 	 * the part of the truncation that must be done without the

[PATCH 3/3] use inode_change_ok for setattr permission checking

[Christoph Hellwig]

[-- Attachment #1: xfs-setattr-use-inode_change_ok --]
[-- Type: text/plain, Size: 3971 bytes --]

Instead of implementing our own checks use inode_change_ok to check for
nessecary permission in setattr.  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.

(First sent on Semptember 29th)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-10-03 22:16:03.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-10-03 22:16:32.000000000 +0200
@@ -70,7 +70,6 @@ xfs_setattr(
 	gid_t			gid=0, igid=0;
 	int			timeflags = 0;
 	struct xfs_dquot	*udqp, *gdqp, *olddquot1, *olddquot2;
-	int			file_owner;
 	int			need_iolock = 1;
 
 	xfs_itrace_entry(ip);
@@ -81,6 +80,10 @@ xfs_setattr(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	code = -inode_change_ok(inode, iattr);
+	if (code)
+		return code;
+
 	olddquot1 = olddquot2 = NULL;
 	udqp = gdqp = NULL;
 
@@ -158,56 +161,6 @@ xfs_setattr(
 
 	xfs_ilock(ip, lock_flags);
 
-	/* boolean: are we the file owner? */
-	file_owner = (current_fsuid() == 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;
-		}
-	}
-
 	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
@@ -224,22 +177,6 @@ xfs_setattr(
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
 		/*
-		 * CAP_CHOWN overrides the following restrictions:
-		 *
-		 * If _POSIX_CHOWN_RESTRICTED is defined, this capability
-		 * shall override the restriction that a process cannot
-		 * change the user ID of a file it owns and the restriction
-		 * that the group ID supplied to the chown() function
-		 * shall be equal to either the group ID or one of the
-		 * supplementary group IDs of the calling process.
-		 */
-		if ((iuid != uid ||
-		     (igid != gid && !in_group_p((gid_t)gid))) &&
-		    !capable(CAP_CHOWN)) {
-			code = XFS_ERROR(EPERM);
-			goto error_return;
-		}
-		/*
 		 * Do a quota reservation only if uid/gid is actually
 		 * going to change.
 		 */
@@ -284,19 +221,6 @@ xfs_setattr(
 	}
 
 	/*
-	 * Change file access or modified times.
-	 */
-	if (mask & (ATTR_ATIME|ATTR_MTIME)) {
-		if (!file_owner) {
-			if ((mask & (ATTR_MTIME_SET|ATTR_ATIME_SET)) &&
-			    !capable(CAP_FOWNER)) {
-				code = XFS_ERROR(EPERM);
-				goto error_return;
-			}
-		}
-	}
-
-	/*
 	 * Now we can make the changes.  Before we join the inode
 	 * to the transaction, if ATTR_SIZE is set then take care of
 	 * the part of the truncation that must be done without the

-- 

[PATCH 3/3] use inode_change_ok for setattr permission checking

[Christoph Hellwig]

[-- Attachment #1: xfs-setattr-use-inode_change_ok --]
[-- Type: text/plain, Size: 3971 bytes --]

Instead of implementing our own checks use inode_change_ok to check for
nessecary permission in setattr.  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.

(First sent on Semptember 29th)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-10-03 22:16:03.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-10-03 22:16:32.000000000 +0200
@@ -70,7 +70,6 @@ xfs_setattr(
 	gid_t			gid=0, igid=0;
 	int			timeflags = 0;
 	struct xfs_dquot	*udqp, *gdqp, *olddquot1, *olddquot2;
-	int			file_owner;
 	int			need_iolock = 1;
 
 	xfs_itrace_entry(ip);
@@ -81,6 +80,10 @@ xfs_setattr(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	code = -inode_change_ok(inode, iattr);
+	if (code)
+		return code;
+
 	olddquot1 = olddquot2 = NULL;
 	udqp = gdqp = NULL;
 
@@ -158,56 +161,6 @@ xfs_setattr(
 
 	xfs_ilock(ip, lock_flags);
 
-	/* boolean: are we the file owner? */
-	file_owner = (current_fsuid() == 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;
-		}
-	}
-
 	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
@@ -224,22 +177,6 @@ xfs_setattr(
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
 		/*
-		 * CAP_CHOWN overrides the following restrictions:
-		 *
-		 * If _POSIX_CHOWN_RESTRICTED is defined, this capability
-		 * shall override the restriction that a process cannot
-		 * change the user ID of a file it owns and the restriction
-		 * that the group ID supplied to the chown() function
-		 * shall be equal to either the group ID or one of the
-		 * supplementary group IDs of the calling process.
-		 */
-		if ((iuid != uid ||
-		     (igid != gid && !in_group_p((gid_t)gid))) &&
-		    !capable(CAP_CHOWN)) {
-			code = XFS_ERROR(EPERM);
-			goto error_return;
-		}
-		/*
 		 * Do a quota reservation only if uid/gid is actually
 		 * going to change.
 		 */
@@ -284,19 +221,6 @@ xfs_setattr(
 	}
 
 	/*
-	 * Change file access or modified times.
-	 */
-	if (mask & (ATTR_ATIME|ATTR_MTIME)) {
-		if (!file_owner) {
-			if ((mask & (ATTR_MTIME_SET|ATTR_ATIME_SET)) &&
-			    !capable(CAP_FOWNER)) {
-				code = XFS_ERROR(EPERM);
-				goto error_return;
-			}
-		}
-	}
-
-	/*
 	 * Now we can make the changes.  Before we join the inode
 	 * to the transaction, if ATTR_SIZE is set then take care of
 	 * the part of the truncation that must be done without the

-- 

Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Dave Chinner]

On Mon, Oct 27, 2008 at 09:36:40AM -0400, Christoph Hellwig wrote:
> Instead of implementing our own checks use inode_change_ok to check for
> nessecary permission in setattr.  There is a slight change in behaviour
  ^^^^^^^^^
necessary

> 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.

Better to get an error than silently lose them, I think.

> (First sent on Semptember 29th)
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Timothy Shimmin]

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@lst.de>
> 
> 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;
> -		}
> -	}
> -

Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Christoph Hellwig]

On Wed, Oct 29, 2008 at 05:35:11PM +1100, Timothy Shimmin wrote:
> 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.

Yes.  The big differene to the NFSv4 ACL patches is that we use the
standard kernel inode_change_ok routine, which means we are guaranteed
to have the same checks as all other filesystems and get rid of
duplicated code.

Btw, I must also say that I really hate the way the NFSv4 ACL patches make
this filesystem-specific for all filesystems that support the NFSv4
ACLs.  All these permission checks should instead go through
->permission with additional MAY_ flags.

> 
> 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?

SUID is handled in the inode_change_ok bit you quoted earlier.  But we
should add this S_ISGID handling here to XFS, too.

> And also, what is with the ATTR_KILL_* bits?
> Lemme look...

ATTR_KILL_SUID/ATTR_KILL_SGID is a rather special thing added for NFS
which doesn't want to do these locally but only on the server.  Local
filesystems can simply ignore it.

Updated patch below.  Note that the S_ISGID hadling required moving
the ATTR_MODE handling after the ATTR_GID handling.

-- 

use inode_change_ok for setattr permission checking

Instead of implementing our own checks use inode_change_ok to check for
nessecary permission in setattr.  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.

(First sent on Semptember 29th)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-11-10 14:03:59.000000000 +0100
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-11-10 14:25:44.000000000 +0100
@@ -70,7 +70,6 @@ xfs_setattr(
 	gid_t			gid=0, igid=0;
 	int			timeflags = 0;
 	struct xfs_dquot	*udqp, *gdqp, *olddquot1, *olddquot2;
-	int			file_owner;
 	int			need_iolock = 1;
 
 	xfs_itrace_entry(ip);
@@ -81,6 +80,10 @@ xfs_setattr(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	code = -inode_change_ok(inode, iattr);
+	if (code)
+		return code;
+
 	olddquot1 = olddquot2 = NULL;
 	udqp = gdqp = NULL;
 
@@ -158,56 +161,6 @@ xfs_setattr(
 
 	xfs_ilock(ip, lock_flags);
 
-	/* boolean: are we the file owner? */
-	file_owner = (current_fsuid() == 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;
-		}
-	}
-
 	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
@@ -224,22 +177,6 @@ xfs_setattr(
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
 		/*
-		 * CAP_CHOWN overrides the following restrictions:
-		 *
-		 * If _POSIX_CHOWN_RESTRICTED is defined, this capability
-		 * shall override the restriction that a process cannot
-		 * change the user ID of a file it owns and the restriction
-		 * that the group ID supplied to the chown() function
-		 * shall be equal to either the group ID or one of the
-		 * supplementary group IDs of the calling process.
-		 */
-		if ((iuid != uid ||
-		     (igid != gid && !in_group_p((gid_t)gid))) &&
-		    !capable(CAP_CHOWN)) {
-			code = XFS_ERROR(EPERM);
-			goto error_return;
-		}
-		/*
 		 * Do a quota reservation only if uid/gid is actually
 		 * going to change.
 		 */
@@ -284,19 +221,6 @@ xfs_setattr(
 	}
 
 	/*
-	 * Change file access or modified times.
-	 */
-	if (mask & (ATTR_ATIME|ATTR_MTIME)) {
-		if (!file_owner) {
-			if ((mask & (ATTR_MTIME_SET|ATTR_ATIME_SET)) &&
-			    !capable(CAP_FOWNER)) {
-				code = XFS_ERROR(EPERM);
-				goto error_return;
-			}
-		}
-	}
-
-	/*
 	 * Now we can make the changes.  Before we join the inode
 	 * to the transaction, if ATTR_SIZE is set then take care of
 	 * the part of the truncation that must be done without the
@@ -413,20 +337,6 @@ xfs_setattr(
 	}
 
 	/*
-	 * Change file access modes.
-	 */
-	if (mask & ATTR_MODE) {
-		ip->i_d.di_mode &= S_IFMT;
-		ip->i_d.di_mode |= iattr->ia_mode & ~S_IFMT;
-
-		inode->i_mode &= S_IFMT;
-		inode->i_mode |= iattr->ia_mode & ~S_IFMT;
-
-		xfs_trans_log_inode (tp, ip, XFS_ILOG_CORE);
-		timeflags |= XFS_ICHGTIME_CHG;
-	}
-
-	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
 	if (mask & (ATTR_UID|ATTR_GID)) {
@@ -471,6 +381,24 @@ xfs_setattr(
 		timeflags |= XFS_ICHGTIME_CHG;
 	}
 
+	/*
+	 * Change file access modes.
+	 */
+	if (mask & ATTR_MODE) {
+		umode_t mode = iattr->ia_mode;
+
+		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
+			mode &= ~S_ISGID;
+
+		ip->i_d.di_mode &= S_IFMT;
+		ip->i_d.di_mode |= mode & ~S_IFMT;
+
+		inode->i_mode &= S_IFMT;
+		inode->i_mode |= mode & ~S_IFMT;
+
+		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+		timeflags |= XFS_ICHGTIME_CHG;
+	}
 
 	/*
 	 * Change file access or modified times.

Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Wed, Oct 29, 2008 at 05:35:11PM +1100, Timothy Shimmin wrote:
>> 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.
> 
> Yes.  The big differene to the NFSv4 ACL patches is that we use the
> standard kernel inode_change_ok routine, which means we are guaranteed
> to have the same checks as all other filesystems and get rid of
> duplicated code.
> 
> Btw, I must also say that I really hate the way the NFSv4 ACL patches make
> this filesystem-specific for all filesystems that support the NFSv4
> ACLs.  All these permission checks should instead go through
> ->permission with additional MAY_ flags.
> 
Yeah, it would be nice to have more out of the filesystem.

>> 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?
> 
> SUID is handled in the inode_change_ok bit you quoted earlier.  

For some reason I can't see where? Sorry.
I see the sgid clear but not the suid clear. D'oh.


> But we
> should add this S_ISGID handling here to XFS, too.
> 
>> And also, what is with the ATTR_KILL_* bits?
>> Lemme look...
> 
> ATTR_KILL_SUID/ATTR_KILL_SGID is a rather special thing added for NFS
> which doesn't want to do these locally but only on the server.  Local
> filesystems can simply ignore it.
> 
Thanks for the explanation.

> Updated patch below.  Note that the S_ISGID hadling required moving
> the ATTR_MODE handling after the ATTR_GID handling.
> 

So from above we have:

notify_change(struct dentry * dentry, struct iattr * attr):

>         if (inode->i_op && inode->i_op->setattr) {
>                 error = security_inode_setattr(dentry, attr);
>                 if (!error)
>                         error = inode->i_op->setattr(dentry, attr);


>         } else {
>                 error = inode_change_ok(inode, attr);
>                 if (!error)
>                         error = security_inode_setattr(dentry, attr);
>                 if (!error) {
>                         if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>                             (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid))
>                                 error = DQUOT_TRANSFER(inode, attr) ? -EDQUOT : 0;
>                         if (!error)
>                                 error = inode_setattr(inode, attr);
>                 }
>         }

So in our case i_op->setattr -> xfs_setattr
And xfs_setattr will now call inode_change_ok().
So we should have similar code to inode_setattr.

So why do we do sgid clear in inode_setattr() as well as in inode_change_ok()?
Doesn't inode_change_ok propagate the attr->ia_mode change back?

Okay, the SGID clearing for mode setting seems fine.

However, we really should have a QA test for this stuff.
Something is bound to stuff up here.

--Tim