[PATCH 0/5 v2 RESEND] fs: Avoid premature clearing of file capabilities

[PATCH 0/5 v2 RESEND] fs: Avoid premature clearing of file capabilities

[Jan Kara]

Hello,

I'm resending the patches since Al did not respond to the series yet (I've
previously posted it in [1] on Aug 3. Al? If Al does not respond, can you
Andrew please feed to patches to Linus? Thanks.

This patch series is my attempt to fix an issue when user can clear capabilites
of arbitrary file he can look up for example by running chown on it (this got
assigned CVE-2015-1350). The problem is that we call security_inode_killpriv()
before checking permissions in inode_change_ok(). This patch set moves that
call into inode_change_ok() after permissions are checked - the only trouble is
that we need to give dentry instead of inode there and that is not completely
trivial in some cases - I'm still missing a review from Ceph people to verify I
didn't miss anything.

Anyway, I guess the changes are fine for merging. Al, you please merge the
patches? Thanks!

Changes since v1:
* Added acks for XFS and FUSE patches

								Honza

[1] https://www.spinics.net/lists/ceph-devel/msg31650.html

[PATCH 1/5] xfs: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. Propagate dentry down to functions calling inode_change_ok().
This is rather straightforward except for xfs_set_mode() function which
does not have dentry easily available. Luckily that function does not
call inode_change_ok() anyway so we just have to do a little dance with
function prototypes.

Acked-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_file.c  |  2 +-
 fs/xfs/xfs_inode.c |  2 +-
 fs/xfs/xfs_ioctl.c |  2 +-
 fs/xfs/xfs_iops.c  | 94 ++++++++++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_iops.h  |  3 +-
 5 files changed, 68 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index e612a0233710..fcb81ede2cb0 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -984,7 +984,7 @@ xfs_file_fallocate(
 
 		iattr.ia_valid = ATTR_SIZE;
 		iattr.ia_size = new_size;
-		error = xfs_setattr_size(ip, &iattr);
+		error = xfs_vn_setattr_size(file_dentry(file), &iattr);
 		if (error)
 			goto out_unlock;
 	}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e08eaea6327b..df22e1254c98 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1710,7 +1710,7 @@ xfs_inactive_truncate(
 	/*
 	 * Log the inode size first to prevent stale data exposure in the event
 	 * of a system crash before the truncate completes. See the related
-	 * comment in xfs_setattr_size() for details.
+	 * comment in xfs_vn_setattr_size() for details.
 	 */
 	ip->i_d.di_size = 0;
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 96a70fd1f5d6..0d9021f0551e 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -720,7 +720,7 @@ xfs_ioc_space(
 		iattr.ia_valid = ATTR_SIZE;
 		iattr.ia_size = bf->l_start;
 
-		error = xfs_setattr_size(ip, &iattr);
+		error = xfs_vn_setattr_size(file_dentry(filp), &iattr);
 		break;
 	default:
 		ASSERT(0);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b24c3102fa93..ffde761fbeff 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -542,6 +542,30 @@ xfs_setattr_time(
 		inode->i_mtime = iattr->ia_mtime;
 }
 
+static int
+xfs_vn_change_ok(
+	struct dentry	*dentry,
+	struct iattr	*iattr)
+{
+	struct inode		*inode = d_inode(dentry);
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+
+	if (mp->m_flags & XFS_MOUNT_RDONLY)
+		return -EROFS;
+
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return -EIO;
+
+	return inode_change_ok(inode, iattr);
+}
+
+/*
+ * Set non-size attributes of an inode.
+ *
+ * Caution: The caller of this function is responsible for calling
+ * inode_change_ok() or otherwise verifying the change is fine.
+ */
 int
 xfs_setattr_nonsize(
 	struct xfs_inode	*ip,
@@ -558,21 +582,6 @@ xfs_setattr_nonsize(
 	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
 	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
 
-	trace_xfs_setattr(ip);
-
-	/* If acls are being inherited, we already have this checked */
-	if (!(flags & XFS_ATTR_NOACL)) {
-		if (mp->m_flags & XFS_MOUNT_RDONLY)
-			return -EROFS;
-
-		if (XFS_FORCED_SHUTDOWN(mp))
-			return -EIO;
-
-		error = inode_change_ok(inode, iattr);
-		if (error)
-			return error;
-	}
-
 	ASSERT((mask & ATTR_SIZE) == 0);
 
 	/*
@@ -743,8 +752,27 @@ out_dqrele:
 	return error;
 }
 
+int
+xfs_vn_setattr_nonsize(
+	struct dentry		*dentry,
+	struct iattr		*iattr)
+{
+	struct xfs_inode	*ip = XFS_I(d_inode(dentry));
+	int error;
+
+	trace_xfs_setattr(ip);
+
+	error = xfs_vn_change_ok(dentry, iattr);
+	if (error)
+		return error;
+	return xfs_setattr_nonsize(ip, iattr, 0);
+}
+
 /*
  * Truncate file.  Must have write permission and not be a directory.
+ *
+ * Caution: The caller of this function is responsible for calling
+ * inode_change_ok() or otherwise verifying the change is fine.
  */
 int
 xfs_setattr_size(
@@ -759,18 +787,6 @@ xfs_setattr_size(
 	uint			lock_flags = 0;
 	bool			did_zeroing = false;
 
-	trace_xfs_setattr(ip);
-
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return -EROFS;
-
-	if (XFS_FORCED_SHUTDOWN(mp))
-		return -EIO;
-
-	error = inode_change_ok(inode, iattr);
-	if (error)
-		return error;
-
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
 	ASSERT(S_ISREG(inode->i_mode));
@@ -942,16 +958,32 @@ out_trans_cancel:
 	goto out_unlock;
 }
 
+int
+xfs_vn_setattr_size(
+	struct dentry		*dentry,
+	struct iattr		*iattr)
+{
+	struct xfs_inode	*ip = XFS_I(d_inode(dentry));
+	int error;
+
+	trace_xfs_setattr(ip);
+
+	error = xfs_vn_change_ok(dentry, iattr);
+	if (error)
+		return error;
+	return xfs_setattr_size(ip, iattr);
+}
+
 STATIC int
 xfs_vn_setattr(
 	struct dentry		*dentry,
 	struct iattr		*iattr)
 {
-	struct xfs_inode	*ip = XFS_I(d_inode(dentry));
 	int			error;
 
 	if (iattr->ia_valid & ATTR_SIZE) {
-		uint		iolock = XFS_IOLOCK_EXCL;
+		struct xfs_inode	*ip = XFS_I(d_inode(dentry));
+		uint			iolock = XFS_IOLOCK_EXCL;
 
 		xfs_ilock(ip, iolock);
 		error = xfs_break_layouts(d_inode(dentry), &iolock, true);
@@ -959,11 +991,11 @@ xfs_vn_setattr(
 			xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
 			iolock |= XFS_MMAPLOCK_EXCL;
 
-			error = xfs_setattr_size(ip, iattr);
+			error = xfs_vn_setattr_size(dentry, iattr);
 		}
 		xfs_iunlock(ip, iolock);
 	} else {
-		error = xfs_setattr_nonsize(ip, iattr, 0);
+		error = xfs_vn_setattr_nonsize(dentry, iattr);
 	}
 
 	return error;
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index a0f84abb0d09..0259a383721a 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -33,6 +33,7 @@ extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
 extern void xfs_setattr_time(struct xfs_inode *ip, struct iattr *iattr);
 extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
 			       int flags);
-extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap);
+extern int xfs_vn_setattr_nonsize(struct dentry *dentry, struct iattr *vap);
+extern int xfs_vn_setattr_size(struct dentry *dentry, struct iattr *vap);
 
 #endif /* __XFS_IOPS_H__ */
-- 
2.6.6

[PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. ceph_setattr() has the dentry easily available but
__ceph_setattr() is also called from ceph_set_acl() where dentry is not
easily available. Luckily that call path does not need inode_change_ok()
to be called anyway. So reorganize functions a bit so that
inode_change_ok() is called only from paths where dentry is available.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ceph/acl.c   |  5 +++++
 fs/ceph/inode.c | 19 +++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 013151d50069..a2cedfde75eb 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			goto out_free;
 	}
 
+	if (ceph_snap(inode) != CEPH_NOSNAP) {
+		ret = -EROFS;
+		goto out_free;
+	}
+
 	if (new_mode != old_mode) {
 		newattrs.ia_mode = new_mode;
 		newattrs.ia_valid = ATTR_MODE;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index dd3a6dbf71eb..2aa3c0bcf3a5 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	int inode_dirty_flags = 0;
 	bool lock_snap_rwsem = false;
 
-	if (ceph_snap(inode) != CEPH_NOSNAP)
-		return -EROFS;
-
-	err = inode_change_ok(inode, attr);
-	if (err != 0)
-		return err;
-
 	prealloc_cf = ceph_alloc_cap_flush();
 	if (!prealloc_cf)
 		return -ENOMEM;
@@ -2124,7 +2117,17 @@ out_put:
  */
 int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	return __ceph_setattr(d_inode(dentry), attr);
+	struct inode *inode = d_inode(dentry);
+	int err;
+
+	if (ceph_snap(inode) != CEPH_NOSNAP)
+		return -EROFS;
+
+	err = inode_change_ok(inode, attr);
+	if (err != 0)
+		return err;
+
+	return __ceph_setattr(inode, attr);
 }
 
 /*
-- 
2.6.6

[PATCH 3/5] fuse: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. Propagate it down to fuse_do_setattr().

Acked-by: Miklos Szeredi <mszeredi@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fuse/dir.c    | 7 ++++---
 fs/fuse/file.c   | 2 +-
 fs/fuse/fuse_i.h | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index c47b7780ce37..6911bfad7834 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1591,9 +1591,10 @@ int fuse_flush_times(struct inode *inode, struct fuse_file *ff)
  * vmtruncate() doesn't allow for this case, so do the rlimit checking
  * and the actual truncation by hand.
  */
-int fuse_do_setattr(struct inode *inode, struct iattr *attr,
+int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 		    struct file *file)
 {
+	struct inode *inode = d_inode(dentry);
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	FUSE_ARGS(args);
@@ -1707,9 +1708,9 @@ static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 		return -EACCES;
 
 	if (attr->ia_valid & ATTR_FILE)
-		return fuse_do_setattr(inode, attr, attr->ia_file);
+		return fuse_do_setattr(entry, attr, attr->ia_file);
 	else
-		return fuse_do_setattr(inode, attr, NULL);
+		return fuse_do_setattr(entry, attr, NULL);
 }
 
 static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index f394aff59c36..4b9201b7c80b 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2841,7 +2841,7 @@ static void fuse_do_truncate(struct file *file)
 	attr.ia_file = file;
 	attr.ia_valid |= ATTR_FILE;
 
-	fuse_do_setattr(inode, &attr, file);
+	fuse_do_setattr(file_dentry(file), &attr, file);
 }
 
 static inline loff_t fuse_round_up(loff_t off)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index d98d8cc84def..cace5526c6d6 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -958,7 +958,7 @@ bool fuse_write_update_size(struct inode *inode, loff_t pos);
 int fuse_flush_times(struct inode *inode, struct fuse_file *ff);
 int fuse_write_inode(struct inode *inode, struct writeback_control *wbc);
 
-int fuse_do_setattr(struct inode *inode, struct iattr *attr,
+int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 		    struct file *file);
 
 void fuse_set_initialized(struct fuse_conn *fc);
-- 
2.6.6

[PATCH 4/5] fs: Give dentry to inode_change_ok() instead of inode

[Jan Kara]

inode_change_ok() will be resposible for clearing capabilities and IMA
extended attributes and as such will need dentry. Give it as an argument
to inode_change_ok() instead of an inode. Also rename inode_change_ok()
to setattr_prepare() to better relect that it does also some
modifications in addition to checks.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 Documentation/filesystems/porting               |  4 ++--
 drivers/staging/lustre/lustre/llite/llite_lib.c |  2 +-
 fs/9p/vfs_inode.c                               |  2 +-
 fs/9p/vfs_inode_dotl.c                          |  2 +-
 fs/adfs/inode.c                                 |  2 +-
 fs/affs/inode.c                                 |  2 +-
 fs/attr.c                                       | 15 +++++++++------
 fs/btrfs/inode.c                                |  2 +-
 fs/ceph/inode.c                                 |  2 +-
 fs/cifs/inode.c                                 |  4 ++--
 fs/ecryptfs/inode.c                             |  2 +-
 fs/exofs/inode.c                                |  2 +-
 fs/ext2/inode.c                                 |  2 +-
 fs/ext4/inode.c                                 |  2 +-
 fs/f2fs/file.c                                  |  2 +-
 fs/fat/file.c                                   |  2 +-
 fs/fuse/dir.c                                   |  2 +-
 fs/gfs2/inode.c                                 |  2 +-
 fs/hfs/inode.c                                  |  2 +-
 fs/hfsplus/inode.c                              |  2 +-
 fs/hostfs/hostfs_kern.c                         |  2 +-
 fs/hpfs/inode.c                                 |  2 +-
 fs/hugetlbfs/inode.c                            |  2 +-
 fs/jffs2/fs.c                                   |  2 +-
 fs/jfs/file.c                                   |  2 +-
 fs/kernfs/inode.c                               |  2 +-
 fs/libfs.c                                      |  2 +-
 fs/logfs/file.c                                 |  2 +-
 fs/minix/file.c                                 |  2 +-
 fs/ncpfs/inode.c                                |  2 +-
 fs/nfsd/nfsproc.c                               |  8 +++-----
 fs/nilfs2/inode.c                               |  2 +-
 fs/ntfs/inode.c                                 |  2 +-
 fs/ocfs2/dlmfs/dlmfs.c                          |  2 +-
 fs/ocfs2/file.c                                 |  2 +-
 fs/omfs/file.c                                  |  2 +-
 fs/orangefs/inode.c                             |  2 +-
 fs/overlayfs/inode.c                            |  2 +-
 fs/proc/base.c                                  |  2 +-
 fs/proc/generic.c                               |  2 +-
 fs/proc/proc_sysctl.c                           |  2 +-
 fs/ramfs/file-nommu.c                           |  2 +-
 fs/reiserfs/inode.c                             |  2 +-
 fs/sysv/file.c                                  |  2 +-
 fs/ubifs/file.c                                 |  2 +-
 fs/udf/file.c                                   |  2 +-
 fs/ufs/inode.c                                  |  2 +-
 fs/utimes.c                                     |  4 ++--
 fs/xfs/xfs_iops.c                               | 10 ++++------
 include/linux/fs.h                              |  2 +-
 mm/shmem.c                                      |  2 +-
 51 files changed, 67 insertions(+), 68 deletions(-)

diff --git a/Documentation/filesystems/porting b/Documentation/filesystems/porting
index b1bd05ea66b2..ab320c77147a 100644
--- a/Documentation/filesystems/porting
+++ b/Documentation/filesystems/porting
@@ -287,8 +287,8 @@ implementing on-disk size changes.  Start with a copy of the old inode_setattr
 and vmtruncate, and the reorder the vmtruncate + foofs_vmtruncate sequence to
 be in order of zeroing blocks using block_truncate_page or similar helpers,
 size update and on finally on-disk truncation which should not fail.
-inode_change_ok now includes the size checks for ATTR_SIZE and must be called
-in the beginning of ->setattr unconditionally.
+setattr_prepare (which used to be inode_change_ok) now includes the size checks
+for ATTR_SIZE and must be called in the beginning of ->setattr unconditionally.
 
 [mandatory]
 
diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index 546063e728db..fb7b54eff027 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -1192,7 +1192,7 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import)
 		attr->ia_valid |= ATTR_MTIME | ATTR_CTIME;
 	}
 
-	/* POSIX: check before ATTR_*TIME_SET set (from inode_change_ok) */
+	/* POSIX: check before ATTR_*TIME_SET set (from setattr_prepare) */
 	if (attr->ia_valid & TIMES_SET_FLAGS) {
 		if ((!uid_eq(current_fsuid(), inode->i_uid)) &&
 		    !capable(CFS_CAP_FOWNER))
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 8b1999b528e9..aa639bb1f289 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1094,7 +1094,7 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct p9_wstat wstat;
 
 	p9_debug(P9_DEBUG_VFS, "\n");
-	retval = inode_change_ok(d_inode(dentry), iattr);
+	retval = setattr_prepare(dentry, iattr);
 	if (retval)
 		return retval;
 
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index eeabcb0bad12..0e6ad3019711 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -558,7 +558,7 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
 
 	p9_debug(P9_DEBUG_VFS, "\n");
 
-	retval = inode_change_ok(inode, iattr);
+	retval = setattr_prepare(dentry, iattr);
 	if (retval)
 		return retval;
 
diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c
index 335055d828e4..f57baaa511aa 100644
--- a/fs/adfs/inode.c
+++ b/fs/adfs/inode.c
@@ -303,7 +303,7 @@ adfs_notify_change(struct dentry *dentry, struct iattr *attr)
 	unsigned int ia_valid = attr->ia_valid;
 	int error;
 	
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 
 	/*
 	 * we can't change the UID or GID of any file -
diff --git a/fs/affs/inode.c b/fs/affs/inode.c
index 0fdb0f5b2239..1aa243502c7f 100644
--- a/fs/affs/inode.c
+++ b/fs/affs/inode.c
@@ -219,7 +219,7 @@ affs_notify_change(struct dentry *dentry, struct iattr *attr)
 
 	pr_debug("notify_change(%lu,0x%x)\n", inode->i_ino, attr->ia_valid);
 
-	error = inode_change_ok(inode,attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		goto out;
 
diff --git a/fs/attr.c b/fs/attr.c
index 42bb42bb3c72..5c45909ea204 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -17,19 +17,22 @@
 #include <linux/ima.h>
 
 /**
- * inode_change_ok - check if attribute changes to an inode are allowed
- * @inode:	inode to check
+ * setattr_prepare - check if attribute changes to a dentry are allowed
+ * @dentry:	dentry to check
  * @attr:	attributes to change
  *
  * Check if we are allowed to change the attributes contained in @attr
- * in the given inode.  This includes the normal unix access permission
- * checks, as well as checks for rlimits and others.
+ * in the given dentry.  This includes the normal unix access permission
+ * checks, as well as checks for rlimits and others. The function also clears
+ * SGID bit from mode if user is not allowed to set it. Also file capabilities
+ * and IMA extended attributes are cleared if ATTR_KILL_PRIV is set.
  *
  * Should be called as the first thing in ->setattr implementations,
  * possibly after taking additional locks.
  */
-int inode_change_ok(const struct inode *inode, struct iattr *attr)
+int setattr_prepare(struct dentry *dentry, struct iattr *attr)
 {
+	struct inode *inode = d_inode(dentry);
 	unsigned int ia_valid = attr->ia_valid;
 
 	/*
@@ -79,7 +82,7 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr)
 
 	return 0;
 }
-EXPORT_SYMBOL(inode_change_ok);
+EXPORT_SYMBOL(setattr_prepare);
 
 /**
  * inode_newsize_ok - may this inode be truncated to a given size
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e6811c42e41e..34bb30ba72c4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5072,7 +5072,7 @@ static int btrfs_setattr(struct dentry *dentry, struct iattr *attr)
 	if (btrfs_root_readonly(root))
 		return -EROFS;
 
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 2aa3c0bcf3a5..082e82dcbaa4 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2123,7 +2123,7 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 	if (ceph_snap(inode) != CEPH_NOSNAP)
 		return -EROFS;
 
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err != 0)
 		return err;
 
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index b87efd0c92d6..13cf507d1423 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2154,7 +2154,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM)
 		attrs->ia_valid |= ATTR_FORCE;
 
-	rc = inode_change_ok(inode, attrs);
+	rc = setattr_prepare(direntry, attrs);
 	if (rc < 0)
 		goto out;
 
@@ -2294,7 +2294,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM)
 		attrs->ia_valid |= ATTR_FORCE;
 
-	rc = inode_change_ok(inode, attrs);
+	rc = setattr_prepare(direntry, attrs);
 	if (rc < 0) {
 		free_xid(xid);
 		return rc;
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 9d153b6a1d72..5ffba186f352 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -927,7 +927,7 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia)
 	}
 	mutex_unlock(&crypt_stat->cs_mutex);
 
-	rc = inode_change_ok(inode, ia);
+	rc = setattr_prepare(dentry, ia);
 	if (rc)
 		goto out;
 	if (ia->ia_valid & ATTR_SIZE) {
diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index 9dc4c6dbf3c9..5e68daee5fe4 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -1034,7 +1034,7 @@ int exofs_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (unlikely(error))
 		return error;
 
-	error = inode_change_ok(inode, iattr);
+	error = setattr_prepare(dentry, iattr);
 	if (unlikely(error))
 		return error;
 
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index d5c7d09919f3..65c077d9a5e9 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1580,7 +1580,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, iattr);
+	error = setattr_prepare(dentry, iattr);
 	if (error)
 		return error;
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c6ea25a190f8..b27491e885c4 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5073,7 +5073,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	int orphan = 0;
 	const unsigned int ia_valid = attr->ia_valid;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 47abb96098e4..fd8bb4457f18 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -680,7 +680,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int err;
 
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/fat/file.c b/fs/fat/file.c
index f70185668832..c09ab4e108e5 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -450,7 +450,7 @@ int fat_setattr(struct dentry *dentry, struct iattr *attr)
 			attr->ia_valid &= ~TIMES_SET_FLAGS;
 	}
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	attr->ia_valid = ia_valid;
 	if (error) {
 		if (sbi->options.quiet)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 6911bfad7834..b23502174682 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1609,7 +1609,7 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 	if (!(fc->flags & FUSE_DEFAULT_PERMISSIONS))
 		attr->ia_valid |= ATTR_FORCE;
 
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index e4da0ecd3285..d693af5ca5bc 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1932,7 +1932,7 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		goto out;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		goto out;
 
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index c6a32415735b..09cce23864da 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -605,7 +605,7 @@ int hfs_inode_setattr(struct dentry *dentry, struct iattr * attr)
 	struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
 	int error;
 
-	error = inode_change_ok(inode, attr); /* basic permission checks */
+	error = setattr_prepare(dentry, attr); /* basic permission checks */
 	if (error)
 		return error;
 
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 19462d773fe2..c43ef397a3aa 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -245,7 +245,7 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 90e46cd752fe..44aa96ba1df8 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -812,7 +812,7 @@ static int hostfs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	int fd = HOSTFS_I(inode)->fd;
 
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/hpfs/inode.c b/fs/hpfs/inode.c
index 1f3c6d76200b..b9c724ed1e7e 100644
--- a/fs/hpfs/inode.c
+++ b/fs/hpfs/inode.c
@@ -273,7 +273,7 @@ int hpfs_setattr(struct dentry *dentry, struct iattr *attr)
 	if ((attr->ia_valid & ATTR_SIZE) && attr->ia_size > inode->i_size)
 		goto out_unlock;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		goto out_unlock;
 
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 4ea71eba40a5..fb3312f2c861 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -672,7 +672,7 @@ static int hugetlbfs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	BUG_ON(!inode);
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index ae2ebb26b446..3773b24b4db0 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -193,7 +193,7 @@ int jffs2_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct inode *inode = d_inode(dentry);
 	int rc;
 
-	rc = inode_change_ok(inode, iattr);
+	rc = setattr_prepare(dentry, iattr);
 	if (rc)
 		return rc;
 
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 7f1a585a0a94..cf62037b8a04 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -103,7 +103,7 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct inode *inode = d_inode(dentry);
 	int rc;
 
-	rc = inode_change_ok(inode, iattr);
+	rc = setattr_prepare(dentry, iattr);
 	if (rc)
 		return rc;
 
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 63b925d5ba1e..df21f5b75549 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -122,7 +122,7 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
 		return -EINVAL;
 
 	mutex_lock(&kernfs_mutex);
-	error = inode_change_ok(inode, iattr);
+	error = setattr_prepare(dentry, iattr);
 	if (error)
 		goto out;
 
diff --git a/fs/libfs.c b/fs/libfs.c
index 74dc8b9e7f53..2b3c3ae70153 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -394,7 +394,7 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, iattr);
+	error = setattr_prepare(dentry, iattr);
 	if (error)
 		return error;
 
diff --git a/fs/logfs/file.c b/fs/logfs/file.c
index f01ddfb1a03b..5d9fe466bbc9 100644
--- a/fs/logfs/file.c
+++ b/fs/logfs/file.c
@@ -244,7 +244,7 @@ static int logfs_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int err = 0;
 
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/minix/file.c b/fs/minix/file.c
index 94f0eb9a6e2c..a6a4797aa0d4 100644
--- a/fs/minix/file.c
+++ b/fs/minix/file.c
@@ -26,7 +26,7 @@ static int minix_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/ncpfs/inode.c b/fs/ncpfs/inode.c
index 1af15fcbe57b..f6cf4c7e92b1 100644
--- a/fs/ncpfs/inode.c
+++ b/fs/ncpfs/inode.c
@@ -884,7 +884,7 @@ int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
 	/* ageing the dentry to force validation */
 	ncp_age_dentry(server, dentry);
 
-	result = inode_change_ok(inode, attr);
+	result = setattr_prepare(dentry, attr);
 	if (result < 0)
 		goto out;
 
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index e9214768cde9..08188743db53 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -74,10 +74,10 @@ nfsd_proc_setattr(struct svc_rqst *rqstp, struct nfsd_sattrargs *argp,
 	 * which only requires access, and "set-[ac]time-to-X" which
 	 * requires ownership.
 	 * So if it looks like it might be "set both to the same time which
-	 * is close to now", and if inode_change_ok fails, then we
+	 * is close to now", and if setattr_prepare fails, then we
 	 * convert to "set to now" instead of "set to explicit time"
 	 *
-	 * We only call inode_change_ok as the last test as technically
+	 * We only call setattr_prepare as the last test as technically
 	 * it is not an interface that we should be using.
 	 */
 #define BOTH_TIME_SET (ATTR_ATIME_SET | ATTR_MTIME_SET)
@@ -92,17 +92,15 @@ nfsd_proc_setattr(struct svc_rqst *rqstp, struct nfsd_sattrargs *argp,
 		 * request is.  We require it be within 30 minutes of now.
 		 */
 		time_t delta = iap->ia_atime.tv_sec - get_seconds();
-		struct inode *inode;
 
 		nfserr = fh_verify(rqstp, fhp, 0, NFSD_MAY_NOP);
 		if (nfserr)
 			goto done;
-		inode = d_inode(fhp->fh_dentry);
 
 		if (delta < 0)
 			delta = -delta;
 		if (delta < MAX_TOUCH_TIME_ERROR &&
-		    inode_change_ok(inode, iap) != 0) {
+		    setattr_prepare(fhp->fh_dentry, iap) != 0) {
 			/*
 			 * Turn off ATTR_[AM]TIME_SET but leave ATTR_[AM]TIME.
 			 * This will cause notify_change to set these times
diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index af04f553d7c9..402c325e0467 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -829,7 +829,7 @@ int nilfs_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct super_block *sb = inode->i_sb;
 	int err;
 
-	err = inode_change_ok(inode, iattr);
+	err = setattr_prepare(dentry, iattr);
 	if (err)
 		return err;
 
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index e01287c964a8..9d7a44872df5 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2893,7 +2893,7 @@ int ntfs_setattr(struct dentry *dentry, struct iattr *attr)
 	int err;
 	unsigned int ia_valid = attr->ia_valid;
 
-	err = inode_change_ok(vi, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		goto out;
 	/* We do not support NTFS ACLs yet. */
diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
index ef474cdd6404..6a4b21654037 100644
--- a/fs/ocfs2/dlmfs/dlmfs.c
+++ b/fs/ocfs2/dlmfs/dlmfs.c
@@ -211,7 +211,7 @@ static int dlmfs_file_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 
 	attr->ia_valid &= ~ATTR_SIZE;
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 4e7b0dc22450..1ab3657242e8 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1155,7 +1155,7 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
 	if (!(attr->ia_valid & OCFS2_VALID_ATTRS))
 		return 0;
 
-	status = inode_change_ok(inode, attr);
+	status = setattr_prepare(dentry, attr);
 	if (status)
 		return status;
 
diff --git a/fs/omfs/file.c b/fs/omfs/file.c
index d9e26cfbb793..bf83e6644333 100644
--- a/fs/omfs/file.c
+++ b/fs/omfs/file.c
@@ -349,7 +349,7 @@ static int omfs_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 28a0557a69be..cff00ebac03a 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -219,7 +219,7 @@ int orangefs_setattr(struct dentry *dentry, struct iattr *iattr)
 		     "orangefs_setattr: called on %s\n",
 		     dentry->d_name.name);
 
-	ret = inode_change_ok(inode, iattr);
+	ret = setattr_prepare(dentry, iattr);
 	if (ret)
 		goto out;
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index c75625c1efa3..251e5253f2c1 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -53,7 +53,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 	 * inode_newsize_ok() will always check against MAX_LFS_FILESIZE and not
 	 * check for a swapfile (which this won't be anyway).
 	 */
-	err = inode_change_ok(dentry->d_inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ac0df4dde823..2212a93263c4 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -709,7 +709,7 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr)
 	if (attr->ia_valid & ATTR_MODE)
 		return -EPERM;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index c633476616e0..23ff30e3c1e7 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -105,7 +105,7 @@ static int proc_notify_change(struct dentry *dentry, struct iattr *iattr)
 	struct proc_dir_entry *de = PDE(inode);
 	int error;
 
-	error = inode_change_ok(inode, iattr);
+	error = setattr_prepare(dentry, iattr);
 	if (error)
 		return error;
 
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 1b93650dda2f..268fcf592b0e 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -754,7 +754,7 @@ static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr)
 	if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		return -EPERM;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index be3ddd189cd4..2bcbf4e77982 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -169,7 +169,7 @@ static int ramfs_nommu_setattr(struct dentry *dentry, struct iattr *ia)
 	int ret = 0;
 
 	/* POSIX UID/GID verification for setting inode attributes */
-	ret = inode_change_ok(inode, ia);
+	ret = setattr_prepare(dentry, ia);
 	if (ret)
 		return ret;
 
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index c2c59f9ff04b..cb7f518d37ae 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -3312,7 +3312,7 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
 	unsigned int ia_valid;
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/sysv/file.c b/fs/sysv/file.c
index 82ddc09061e2..7ba997e31aeb 100644
--- a/fs/sysv/file.c
+++ b/fs/sysv/file.c
@@ -33,7 +33,7 @@ static int sysv_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 7bbf420d1289..b0a6a53263f3 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1262,7 +1262,7 @@ int ubifs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	dbg_gen("ino %lu, mode %#x, ia_valid %#x",
 		inode->i_ino, inode->i_mode, attr->ia_valid);
-	err = inode_change_ok(inode, attr);
+	err = setattr_prepare(dentry, attr);
 	if (err)
 		return err;
 
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 632570617327..2d61d9827e6f 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -247,7 +247,7 @@ static int udf_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_inode(dentry);
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 9f49431e798d..e4a4d248a0f5 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -1208,7 +1208,7 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr)
 	unsigned int ia_valid = attr->ia_valid;
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
diff --git a/fs/utimes.c b/fs/utimes.c
index 794f5f5b1fb5..c84b26e46bc5 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -81,7 +81,7 @@ static int utimes_common(struct path *path, struct timespec *times)
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
 		/*
-		 * Tell inode_change_ok(), that this is an explicit time
+		 * Tell setattr_prepare(), that this is an explicit time
 		 * update, even if neither ATTR_ATIME_SET nor ATTR_MTIME_SET
 		 * were used.
 		 */
@@ -90,7 +90,7 @@ static int utimes_common(struct path *path, struct timespec *times)
 		/*
 		 * If times is NULL (or both times are UTIME_NOW),
 		 * then we need to check permissions, because
-		 * inode_change_ok() won't do it.
+		 * setattr_prepare() won't do it.
 		 */
 		error = -EPERM;
                 if (IS_IMMUTABLE(inode))
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ffde761fbeff..ba99803eba98 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -547,9 +547,7 @@ xfs_vn_change_ok(
 	struct dentry	*dentry,
 	struct iattr	*iattr)
 {
-	struct inode		*inode = d_inode(dentry);
-	struct xfs_inode	*ip = XFS_I(inode);
-	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_mount	*mp = XFS_I(d_inode(dentry))->i_mount;
 
 	if (mp->m_flags & XFS_MOUNT_RDONLY)
 		return -EROFS;
@@ -557,14 +555,14 @@ xfs_vn_change_ok(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	return inode_change_ok(inode, iattr);
+	return setattr_prepare(dentry, iattr);
 }
 
 /*
  * Set non-size attributes of an inode.
  *
  * Caution: The caller of this function is responsible for calling
- * inode_change_ok() or otherwise verifying the change is fine.
+ * setattr_prepare() or otherwise verifying the change is fine.
  */
 int
 xfs_setattr_nonsize(
@@ -772,7 +770,7 @@ xfs_vn_setattr_nonsize(
  * Truncate file.  Must have write permission and not be a directory.
  *
  * Caution: The caller of this function is responsible for calling
- * inode_change_ok() or otherwise verifying the change is fine.
+ * setattr_prepare() or otherwise verifying the change is fine.
  */
 int
 xfs_setattr_size(
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 901e25d495cc..efdcac2fb768 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2995,7 +2995,7 @@ extern int buffer_migrate_page(struct address_space *,
 #define buffer_migrate_page NULL
 #endif
 
-extern int inode_change_ok(const struct inode *, struct iattr *);
+extern int setattr_prepare(struct dentry *, struct iattr *);
 extern int inode_newsize_ok(const struct inode *, loff_t offset);
 extern void setattr_copy(struct inode *inode, const struct iattr *attr);
 
diff --git a/mm/shmem.c b/mm/shmem.c
index fd8b2b5741b1..6e714f12a24a 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -959,7 +959,7 @@ static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
 	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
 	int error;
 
-	error = inode_change_ok(inode, attr);
+	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
-- 
2.6.6

[PATCH 5/5] fs: Avoid premature clearing of capabilities

[Jan Kara]

Currently, notify_change() clears capabilities or IMA attributes by
calling security_inode_killpriv() before calling into ->setattr. Thus it
happens before any other permission checks in inode_change_ok() and user
is thus allowed to trigger clearing of capabilities or IMA attributes
for any file he can look up e.g. by calling chown for that file. This is
unexpected and can lead to user DoSing a system.

Fix the problem by calling security_inode_killpriv() at the end of
inode_change_ok() instead of from notify_change(). At that moment we are
sure user has permissions to do the requested change.

References: CVE-2015-1350
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/attr.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 5c45909ea204..83c8430d908f 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -47,7 +47,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
 
 	/* If force is set do it anyway. */
 	if (ia_valid & ATTR_FORCE)
-		return 0;
+		goto kill_priv;
 
 	/* Make sure a caller can chown. */
 	if ((ia_valid & ATTR_UID) &&
@@ -80,6 +80,16 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
 			return -EPERM;
 	}
 
+kill_priv:
+	/* User has permission for the change */
+	if (ia_valid & ATTR_KILL_PRIV) {
+		int error;
+
+		error = security_inode_killpriv(dentry);
+		if (error)
+			return error;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(setattr_prepare);
@@ -220,13 +230,11 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	if (!(ia_valid & ATTR_MTIME_SET))
 		attr->ia_mtime = now;
 	if (ia_valid & ATTR_KILL_PRIV) {
-		attr->ia_valid &= ~ATTR_KILL_PRIV;
-		ia_valid &= ~ATTR_KILL_PRIV;
 		error = security_inode_need_killpriv(dentry);
-		if (error > 0)
-			error = security_inode_killpriv(dentry);
-		if (error)
+		if (error < 0)
 			return error;
+		if (error == 0)
+			ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
 	}
 
 	/*
-- 
2.6.6

Re: [PATCH 0/5 v2 RESEND] fs: Avoid premature clearing of file capabilities

[Christoph Hellwig]

On Mon, Sep 19, 2016 at 05:30:54PM +0200, Jan Kara wrote:
> Hello,
> 
> I'm resending the patches since Al did not respond to the series yet (I've
> previously posted it in [1] on Aug 3. Al? If Al does not respond, can you
> Andrew please feed to patches to Linus? Thanks.

Yes, please get the series in.  I thought I reviewed it before, but
here it comes again:

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

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jeff Layton]

On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote:
> To avoid clearing of capabilities or security related extended
> attributes too early, inode_change_ok() will need to take dentry instead
> of inode. ceph_setattr() has the dentry easily available but
> __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> easily available. Luckily that call path does not need inode_change_ok()
> to be called anyway. So reorganize functions a bit so that
> inode_change_ok() is called only from paths where dentry is available.
> 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ceph/acl.c   |  5 +++++
>  fs/ceph/inode.c | 19 +++++++++++--------
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> index 013151d50069..a2cedfde75eb 100644
> --- a/fs/ceph/acl.c
> +++ b/fs/ceph/acl.c
> @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  			goto out_free;
> >  	}
>  
> > +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> > +		ret = -EROFS;
> > +		goto out_free;
> > +	}
> +

So to make sure I understand: What's the expected behavior when someone
changes the ACL in such a way that the mode gets changed? Should
security_inode_killpriv be getting called in that case? If so, where
does that happen, as I don't see notify_change being called in this
codepath?

>  	if (new_mode != old_mode) {
> >  		newattrs.ia_mode = new_mode;
> >  		newattrs.ia_valid = ATTR_MODE;
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index dd3a6dbf71eb..2aa3c0bcf3a5 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> >  	int inode_dirty_flags = 0;
> >  	bool lock_snap_rwsem = false;
>  
> > -	if (ceph_snap(inode) != CEPH_NOSNAP)
> > -		return -EROFS;
> -
> > -	err = inode_change_ok(inode, attr);
> > -	if (err != 0)
> > -		return err;
> -
> >  	prealloc_cf = ceph_alloc_cap_flush();
> >  	if (!prealloc_cf)
> >  		return -ENOMEM;
> @@ -2124,7 +2117,17 @@ out_put:
>   */
>  int ceph_setattr(struct dentry *dentry, struct iattr *attr)
>  {
> > -	return __ceph_setattr(d_inode(dentry), attr);
> > +	struct inode *inode = d_inode(dentry);
> > +	int err;
> +
> > +	if (ceph_snap(inode) != CEPH_NOSNAP)
> > +		return -EROFS;
> +
> > +	err = inode_change_ok(inode, attr);
> > +	if (err != 0)
> > +		return err;
> +
> > +	return __ceph_setattr(inode, attr);
>  }
>  
>  /*

Looks reasonable though:

Acked-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

On Mon 19-09-16 14:57:02, Jeff Layton wrote:
> On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote:
> > To avoid clearing of capabilities or security related extended
> > attributes too early, inode_change_ok() will need to take dentry instead
> > of inode. ceph_setattr() has the dentry easily available but
> > __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> > easily available. Luckily that call path does not need inode_change_ok()
> > to be called anyway. So reorganize functions a bit so that
> > inode_change_ok() is called only from paths where dentry is available.
> > 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > �fs/ceph/acl.c���|��5 +++++
> > �fs/ceph/inode.c | 19 +++++++++++--------
> > �2 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > index 013151d50069..a2cedfde75eb 100644
> > --- a/fs/ceph/acl.c
> > +++ b/fs/ceph/acl.c
> > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > > �			goto out_free;
> > > �	}
> > �
> > > +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> > > +		ret = -EROFS;
> > > +		goto out_free;
> > > +	}
> > +
> 
> So to make sure I understand: What's the expected behavior when someone
> changes the ACL in such a way that the mode gets changed? Should
> security_inode_killpriv be getting called in that case? If so, where
> does that happen, as I don't see notify_change being called in this
> codepath?

No. security_inode_killpriv() is supposed to be called when either contents
of the file changes (truncate, write) or when owner of the file changes. It
is not called for permission changes.

...
> Looks reasonable though:
> 
> Acked-by: Jeff Layton <jlayton@redhat.com>

Thanks for review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

On Mon 19-09-16 14:57:02, Jeff Layton wrote:
> On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote:
> > To avoid clearing of capabilities or security related extended
> > attributes too early, inode_change_ok() will need to take dentry instead
> > of inode. ceph_setattr() has the dentry easily available but
> > __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> > easily available. Luckily that call path does not need inode_change_ok()
> > to be called anyway. So reorganize functions a bit so that
> > inode_change_ok() is called only from paths where dentry is available.
> > 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ceph/acl.c   |  5 +++++
> >  fs/ceph/inode.c | 19 +++++++++++--------
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > index 013151d50069..a2cedfde75eb 100644
> > --- a/fs/ceph/acl.c
> > +++ b/fs/ceph/acl.c
> > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > >  			goto out_free;
> > >  	}
> >  
> > > +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> > > +		ret = -EROFS;
> > > +		goto out_free;
> > > +	}
> > +
> 
> So to make sure I understand: What's the expected behavior when someone
> changes the ACL in such a way that the mode gets changed? Should
> security_inode_killpriv be getting called in that case? If so, where
> does that happen, as I don't see notify_change being called in this
> codepath?

No. security_inode_killpriv() is supposed to be called when either contents
of the file changes (truncate, write) or when owner of the file changes. It
is not called for permission changes.

...
> Looks reasonable though:
> 
> Acked-by: Jeff Layton <jlayton@redhat.com>

Thanks for review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jeff Layton]

On Thu, 2016-09-22 at 10:50 +0200, Jan Kara wrote:
> On Mon 19-09-16 14:57:02, Jeff Layton wrote:
> > 
> > On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote:
> > > 
> > > To avoid clearing of capabilities or security related extended
> > > attributes too early, inode_change_ok() will need to take dentry instead
> > > of inode. ceph_setattr() has the dentry easily available but
> > > __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> > > easily available. Luckily that call path does not need inode_change_ok()
> > > to be called anyway. So reorganize functions a bit so that
> > > inode_change_ok() is called only from paths where dentry is available.
> > > 
> > > > 
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/ceph/acl.c   |  5 +++++
> > >  fs/ceph/inode.c | 19 +++++++++++--------
> > >  2 files changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > > index 013151d50069..a2cedfde75eb 100644
> > > --- a/fs/ceph/acl.c
> > > +++ b/fs/ceph/acl.c
> > > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > > > 
> > > >  			goto out_free;
> > > >  	}
> > >  
> > > > 
> > > > +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> > > > +		ret = -EROFS;
> > > > +		goto out_free;
> > > > +	}
> > > +
> > 
> > So to make sure I understand: What's the expected behavior when someone
> > changes the ACL in such a way that the mode gets changed? Should
> > security_inode_killpriv be getting called in that case? If so, where
> > does that happen, as I don't see notify_change being called in this
> > codepath?
> 
> No. security_inode_killpriv() is supposed to be called when either contents
> of the file changes (truncate, write) or when owner of the file changes. It
> is not called for permission changes.
> 
> ...

Huh, ok...

I would have thought that changing the mode of the file should remove
capabilities though. Consider:

Suppose we have a binary with extra file capabilities that is only
executable by group owner. Attacker is able to change the permissions
such that it's executable by world, and now anyone can get that
capability since it wasn't revoked. IOW, I wonder if we may be trying
to follow suit a little too closely with how the setuid bit works.

When you call chmod(), you have to reinstate the setuid bit anyway if
you want to keep it so not revoking it is ok there. Capabilities are
stored separately though, so you don't need to make the same conscious
decision to keep them there. Maybe the kernel should be revoking them
when the mode changes?

> > 
> > Looks reasonable though:
> > 
> > Acked-by: Jeff Layton <jlayton@redhat.com>
> 
> Thanks for review!
> 
> 								Honza

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Yan, Zheng]

> On 19 Sep 2016, at 23:30, Jan Kara <jack@suse.cz> wrote:
> 
> To avoid clearing of capabilities or security related extended
> attributes too early, inode_change_ok() will need to take dentry instead
> of inode. ceph_setattr() has the dentry easily available but
> __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> easily available. Luckily that call path does not need inode_change_ok()
> to be called anyway. So reorganize functions a bit so that
> inode_change_ok() is called only from paths where dentry is available.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ceph/acl.c   |  5 +++++
> fs/ceph/inode.c | 19 +++++++++++--------
> 2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> index 013151d50069..a2cedfde75eb 100644
> --- a/fs/ceph/acl.c
> +++ b/fs/ceph/acl.c
> @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> 			goto out_free;
> 	}
> 
> +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> +		ret = -EROFS;
> +		goto out_free;
> +	}
> +
> 	if (new_mode != old_mode) {
> 		newattrs.ia_mode = new_mode;
> 		newattrs.ia_valid = ATTR_MODE;
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index dd3a6dbf71eb..2aa3c0bcf3a5 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> 	int inode_dirty_flags = 0;
> 	bool lock_snap_rwsem = false;
> 
> -	if (ceph_snap(inode) != CEPH_NOSNAP)
> -		return -EROFS;
> -
> -	err = inode_change_ok(inode, attr);
> -	if (err != 0)
> -		return err;
> -
> 	prealloc_cf = ceph_alloc_cap_flush();
> 	if (!prealloc_cf)
> 		return -ENOMEM;
> @@ -2124,7 +2117,17 @@ out_put:
>  */
> int ceph_setattr(struct dentry *dentry, struct iattr *attr)
> {
> -	return __ceph_setattr(d_inode(dentry), attr);
> +	struct inode *inode = d_inode(dentry);
> +	int err;
> +
> +	if (ceph_snap(inode) != CEPH_NOSNAP)
> +		return -EROFS;
> +
> +	err = inode_change_ok(inode, attr);
> +	if (err != 0)
> +		return err;
> +
> +	return __ceph_setattr(inode, attr);
> }

Acked-by: 'Yan, Zheng’ <zyan@redhat.com>
> 
> /*
> -- 
> 2.6.6
> 

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

On Thu 22-09-16 13:20:48, Jeff Layton wrote:
> On Thu, 2016-09-22 at 10:50 +0200, Jan Kara wrote:
> > On Mon 19-09-16 14:57:02, Jeff Layton wrote:
> > > 
> > > On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote:
> > > > 
> > > > To avoid clearing of capabilities or security related extended
> > > > attributes too early, inode_change_ok() will need to take dentry instead
> > > > of inode. ceph_setattr() has the dentry easily available but
> > > > __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> > > > easily available. Luckily that call path does not need inode_change_ok()
> > > > to be called anyway. So reorganize functions a bit so that
> > > > inode_change_ok() is called only from paths where dentry is available.
> > > > 
> > > > > 
> > > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > > �fs/ceph/acl.c���|��5 +++++
> > > > �fs/ceph/inode.c | 19 +++++++++++--------
> > > > �2 files changed, 16 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > > > index 013151d50069..a2cedfde75eb 100644
> > > > --- a/fs/ceph/acl.c
> > > > +++ b/fs/ceph/acl.c
> > > > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > > > > 
> > > > > �			goto out_free;
> > > > > �	}
> > > > �
> > > > > 
> > > > > +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> > > > > +		ret = -EROFS;
> > > > > +		goto out_free;
> > > > > +	}
> > > > +
> > > 
> > > So to make sure I understand: What's the expected behavior when someone
> > > changes the ACL in such a way that the mode gets changed? Should
> > > security_inode_killpriv be getting called in that case? If so, where
> > > does that happen, as I don't see notify_change being called in this
> > > codepath?
> > 
> > No. security_inode_killpriv() is supposed to be called when either contents
> > of the file changes (truncate, write) or when owner of the file changes. It
> > is not called for permission changes.
> > 
> > ...
> 
> Huh, ok...
> 
> I would have thought that changing the mode of the file should remove
> capabilities though. Consider:
> 
> Suppose we have a binary with extra file capabilities that is only
> executable by group owner. Attacker is able to change the permissions
> such that it's executable by world, and now anyone can get that
> capability since it wasn't revoked. IOW, I wonder if we may be trying
> to follow suit a little too closely with how the setuid bit works.
> 
> When you call chmod(), you have to reinstate the setuid bit anyway if
> you want to keep it so not revoking it is ok there. Capabilities are
> stored separately though, so you don't need to make the same conscious
> decision to keep them there. Maybe the kernel should be revoking them
> when the mode changes?

So this is certainly an orthogonal issue and I don't want to complicate
this patch set with it. But yeah, your arguments make some sense -
inadvertedly exposing an excutable with additional capabilities may be an
issue. However we behave like this for a long time so I'm not sure we have
the liberty of changing the behavior. Anyway, I've added
linux-security-module to CC if they have anything to say to this.

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

On Thu 22-09-16 13:20:48, Jeff Layton wrote:
> On Thu, 2016-09-22 at 10:50 +0200, Jan Kara wrote:
> > On Mon 19-09-16 14:57:02, Jeff Layton wrote:
> > > 
> > > On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote:
> > > > 
> > > > To avoid clearing of capabilities or security related extended
> > > > attributes too early, inode_change_ok() will need to take dentry instead
> > > > of inode. ceph_setattr() has the dentry easily available but
> > > > __ceph_setattr() is also called from ceph_set_acl() where dentry is not
> > > > easily available. Luckily that call path does not need inode_change_ok()
> > > > to be called anyway. So reorganize functions a bit so that
> > > > inode_change_ok() is called only from paths where dentry is available.
> > > > 
> > > > > 
> > > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/ceph/acl.c   |  5 +++++
> > > >  fs/ceph/inode.c | 19 +++++++++++--------
> > > >  2 files changed, 16 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > > > index 013151d50069..a2cedfde75eb 100644
> > > > --- a/fs/ceph/acl.c
> > > > +++ b/fs/ceph/acl.c
> > > > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > > > > 
> > > > >  			goto out_free;
> > > > >  	}
> > > >  
> > > > > 
> > > > > +	if (ceph_snap(inode) != CEPH_NOSNAP) {
> > > > > +		ret = -EROFS;
> > > > > +		goto out_free;
> > > > > +	}
> > > > +
> > > 
> > > So to make sure I understand: What's the expected behavior when someone
> > > changes the ACL in such a way that the mode gets changed? Should
> > > security_inode_killpriv be getting called in that case? If so, where
> > > does that happen, as I don't see notify_change being called in this
> > > codepath?
> > 
> > No. security_inode_killpriv() is supposed to be called when either contents
> > of the file changes (truncate, write) or when owner of the file changes. It
> > is not called for permission changes.
> > 
> > ...
> 
> Huh, ok...
> 
> I would have thought that changing the mode of the file should remove
> capabilities though. Consider:
> 
> Suppose we have a binary with extra file capabilities that is only
> executable by group owner. Attacker is able to change the permissions
> such that it's executable by world, and now anyone can get that
> capability since it wasn't revoked. IOW, I wonder if we may be trying
> to follow suit a little too closely with how the setuid bit works.
> 
> When you call chmod(), you have to reinstate the setuid bit anyway if
> you want to keep it so not revoking it is ok there. Capabilities are
> stored separately though, so you don't need to make the same conscious
> decision to keep them there. Maybe the kernel should be revoking them
> when the mode changes?

So this is certainly an orthogonal issue and I don't want to complicate
this patch set with it. But yeah, your arguments make some sense -
inadvertedly exposing an excutable with additional capabilities may be an
issue. However we behave like this for a long time so I'm not sure we have
the liberty of changing the behavior. Anyway, I've added
linux-security-module to CC if they have anything to say to this.

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. ceph_setattr() has the dentry easily available but
__ceph_setattr() is also called from ceph_set_acl() where dentry is not
easily available. Luckily that call path does not need inode_change_ok()
to be called anyway. So reorganize functions a bit so that
inode_change_ok() is called only from paths where dentry is available.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ceph/acl.c   |  5 +++++
 fs/ceph/inode.c | 19 +++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 013151d50069..a2cedfde75eb 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			goto out_free;
 	}
 
+	if (ceph_snap(inode) != CEPH_NOSNAP) {
+		ret = -EROFS;
+		goto out_free;
+	}
+
 	if (new_mode != old_mode) {
 		newattrs.ia_mode = new_mode;
 		newattrs.ia_valid = ATTR_MODE;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index dd3a6dbf71eb..2aa3c0bcf3a5 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	int inode_dirty_flags = 0;
 	bool lock_snap_rwsem = false;
 
-	if (ceph_snap(inode) != CEPH_NOSNAP)
-		return -EROFS;
-
-	err = inode_change_ok(inode, attr);
-	if (err != 0)
-		return err;
-
 	prealloc_cf = ceph_alloc_cap_flush();
 	if (!prealloc_cf)
 		return -ENOMEM;
@@ -2124,7 +2117,17 @@ out_put:
  */
 int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	return __ceph_setattr(d_inode(dentry), attr);
+	struct inode *inode = d_inode(dentry);
+	int err;
+
+	if (ceph_snap(inode) != CEPH_NOSNAP)
+		return -EROFS;
+
+	err = inode_change_ok(inode, attr);
+	if (err != 0)
+		return err;
+
+	return __ceph_setattr(inode, attr);
 }
 
 /*
-- 
2.6.6

[PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. ceph_setattr() has the dentry easily available but
__ceph_setattr() is also called from ceph_set_acl() where dentry is not
easily available. Luckily that call path does not need inode_change_ok()
to be called anyway. So reorganize functions a bit so that
inode_change_ok() is called only from paths where dentry is available.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ceph/acl.c   |  5 +++++
 fs/ceph/inode.c | 19 +++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 013151d50069..a2cedfde75eb 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			goto out_free;
 	}
 
+	if (ceph_snap(inode) != CEPH_NOSNAP) {
+		ret = -EROFS;
+		goto out_free;
+	}
+
 	if (new_mode != old_mode) {
 		newattrs.ia_mode = new_mode;
 		newattrs.ia_valid = ATTR_MODE;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index dd3a6dbf71eb..2aa3c0bcf3a5 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	int inode_dirty_flags = 0;
 	bool lock_snap_rwsem = false;
 
-	if (ceph_snap(inode) != CEPH_NOSNAP)
-		return -EROFS;
-
-	err = inode_change_ok(inode, attr);
-	if (err != 0)
-		return err;
-
 	prealloc_cf = ceph_alloc_cap_flush();
 	if (!prealloc_cf)
 		return -ENOMEM;
@@ -2124,7 +2117,17 @@ out_put:
  */
 int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	return __ceph_setattr(d_inode(dentry), attr);
+	struct inode *inode = d_inode(dentry);
+	int err;
+
+	if (ceph_snap(inode) != CEPH_NOSNAP)
+		return -EROFS;
+
+	err = inode_change_ok(inode, attr);
+	if (err != 0)
+		return err;
+
+	return __ceph_setattr(inode, attr);
 }
 
 /*
-- 
2.6.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. ceph_setattr() has the dentry easily available but
__ceph_setattr() is also called from ceph_set_acl() where dentry is not
easily available. Luckily that call path does not need inode_change_ok()
to be called anyway. So reorganize functions a bit so that
inode_change_ok() is called only from paths where dentry is available.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ceph/acl.c   |  5 +++++
 fs/ceph/inode.c | 19 +++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 013151d50069..a2cedfde75eb 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			goto out_free;
 	}
 
+	if (ceph_snap(inode) != CEPH_NOSNAP) {
+		ret = -EROFS;
+		goto out_free;
+	}
+
 	if (new_mode != old_mode) {
 		newattrs.ia_mode = new_mode;
 		newattrs.ia_valid = ATTR_MODE;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e669cfa9d793..29873cbd6b2b 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1791,13 +1791,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	int inode_dirty_flags = 0;
 	bool lock_snap_rwsem = false;
 
-	if (ceph_snap(inode) != CEPH_NOSNAP)
-		return -EROFS;
-
-	err = inode_change_ok(inode, attr);
-	if (err != 0)
-		return err;
-
 	prealloc_cf = ceph_alloc_cap_flush();
 	if (!prealloc_cf)
 		return -ENOMEM;
@@ -2011,7 +2004,17 @@ out_put:
  */
 int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	return __ceph_setattr(d_inode(dentry), attr);
+	struct inode *inode = d_inode(dentry);
+	int err;
+
+	if (ceph_snap(inode) != CEPH_NOSNAP)
+		return -EROFS;
+
+	err = inode_change_ok(inode, attr);
+	if (err != 0)
+		return err;
+
+	return __ceph_setattr(inode, attr);
 }
 
 /*
-- 
2.6.6

[PATCH 2/5] ceph: Propagate dentry down to inode_change_ok()

[Jan Kara]

To avoid clearing of capabilities or security related extended
attributes too early, inode_change_ok() will need to take dentry instead
of inode. ceph_setattr() has the dentry easily available but
__ceph_setattr() is also called from ceph_set_acl() where dentry is not
easily available. Luckily that call path does not need inode_change_ok()
to be called anyway. So reorganize functions a bit so that
inode_change_ok() is called only from paths where dentry is available.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ceph/acl.c   |  5 +++++
 fs/ceph/inode.c | 19 +++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 013151d50069..a2cedfde75eb 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			goto out_free;
 	}
 
+	if (ceph_snap(inode) != CEPH_NOSNAP) {
+		ret = -EROFS;
+		goto out_free;
+	}
+
 	if (new_mode != old_mode) {
 		newattrs.ia_mode = new_mode;
 		newattrs.ia_valid = ATTR_MODE;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e669cfa9d793..29873cbd6b2b 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1791,13 +1791,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	int inode_dirty_flags = 0;
 	bool lock_snap_rwsem = false;
 
-	if (ceph_snap(inode) != CEPH_NOSNAP)
-		return -EROFS;
-
-	err = inode_change_ok(inode, attr);
-	if (err != 0)
-		return err;
-
 	prealloc_cf = ceph_alloc_cap_flush();
 	if (!prealloc_cf)
 		return -ENOMEM;
@@ -2011,7 +2004,17 @@ out_put:
  */
 int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	return __ceph_setattr(d_inode(dentry), attr);
+	struct inode *inode = d_inode(dentry);
+	int err;
+
+	if (ceph_snap(inode) != CEPH_NOSNAP)
+		return -EROFS;
+
+	err = inode_change_ok(inode, attr);
+	if (err != 0)
+		return err;
+
+	return __ceph_setattr(inode, attr);
 }
 
 /*
-- 
2.6.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs