[PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Jan Kara]

When file permissions are modified via chmod(2) and the user is not in
the owning group or capable of CAP_FSETID, the setgid bit is cleared in
inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
permissions as well as the new ACL, but doesn't clear the setgid bit in
a similar way; this allows to bypass the check in chmod(2).  Fix that.

References: CVE-2016-7097
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/9p/acl.c               | 40 +++++++++++++++++-----------------------
 fs/btrfs/acl.c            |  6 ++----
 fs/ceph/acl.c             |  6 ++----
 fs/ext2/acl.c             | 12 ++++--------
 fs/ext4/acl.c             | 12 ++++--------
 fs/f2fs/acl.c             |  6 ++----
 fs/gfs2/acl.c             | 12 +++---------
 fs/hfsplus/posix_acl.c    |  4 ++--
 fs/jffs2/acl.c            |  9 ++++-----
 fs/jfs/acl.c              |  6 ++----
 fs/ocfs2/acl.c            | 10 ++++------
 fs/orangefs/acl.c         | 15 +++++----------
 fs/posix_acl.c            | 31 +++++++++++++++++++++++++++++++
 fs/reiserfs/xattr_acl.c   |  8 ++------
 fs/xfs/xfs_acl.c          | 13 ++++---------
 include/linux/posix_acl.h |  1 +
 16 files changed, 89 insertions(+), 102 deletions(-)

Another forgotten patch. It was posted on May 27 and Aug 19. Al, can you
please pick it up? Andrew, if Al does not respond, can you please take care
of pushing it to Linus? Thanks!

diff --git a/fs/9p/acl.c b/fs/9p/acl.c
index 5b6a1743ea17..b3c2cc79c20d 100644
--- a/fs/9p/acl.c
+++ b/fs/9p/acl.c
@@ -276,32 +276,26 @@ static int v9fs_xattr_set_acl(const struct xattr_handler *handler,
 	switch (handler->flags) {
 	case ACL_TYPE_ACCESS:
 		if (acl) {
-			umode_t mode = inode->i_mode;
-			retval = posix_acl_equiv_mode(acl, &mode);
-			if (retval < 0)
+			struct iattr iattr;
+
+			retval = posix_acl_update_mode(inode, &iattr.ia_mode, &acl);
+			if (retval)
 				goto err_out;
-			else {
-				struct iattr iattr;
-				if (retval == 0) {
-					/*
-					 * ACL can be represented
-					 * by the mode bits. So don't
-					 * update ACL.
-					 */
-					acl = NULL;
-					value = NULL;
-					size = 0;
-				}
-				/* Updte the mode bits */
-				iattr.ia_mode = ((mode & S_IALLUGO) |
-						 (inode->i_mode & ~S_IALLUGO));
-				iattr.ia_valid = ATTR_MODE;
-				/* FIXME should we update ctime ?
-				 * What is the following setxattr update the
-				 * mode ?
+			if (!acl) {
+				/*
+				 * ACL can be represented
+				 * by the mode bits. So don't
+				 * update ACL.
 				 */
-				v9fs_vfs_setattr_dotl(dentry, &iattr);
+				value = NULL;
+				size = 0;
 			}
+			iattr.ia_valid = ATTR_MODE;
+			/* FIXME should we update ctime ?
+			 * What is the following setxattr update the
+			 * mode ?
+			 */
+			v9fs_vfs_setattr_dotl(dentry, &iattr);
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 53bb7af4e5f0..247b8dfaf6e5 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -79,11 +79,9 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			ret = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (ret < 0)
+			ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (ret)
 				return ret;
-			if (ret == 0)
-				acl = NULL;
 		}
 		ret = 0;
 		break;
diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 4f67227f69a5..d0b6b342dff9 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -95,11 +95,9 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			ret = posix_acl_equiv_mode(acl, &new_mode);
-			if (ret < 0)
+			ret = posix_acl_update_mode(inode, &new_mode, &acl);
+			if (ret)
 				goto out;
-			if (ret == 0)
-				acl = NULL;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
index 42f1d1814083..e725aa0890e0 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -190,15 +190,11 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		case ACL_TYPE_ACCESS:
 			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
 			if (acl) {
-				error = posix_acl_equiv_mode(acl, &inode->i_mode);
-				if (error < 0)
+				error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+				if (error)
 					return error;
-				else {
-					inode->i_ctime = CURRENT_TIME_SEC;
-					mark_inode_dirty(inode);
-					if (error == 0)
-						acl = NULL;
-				}
+				inode->i_ctime = CURRENT_TIME_SEC;
+				mark_inode_dirty(inode);
 			}
 			break;
 
diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index c6601a476c02..dfa519979038 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -193,15 +193,11 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 	case ACL_TYPE_ACCESS:
 		name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (error < 0)
+			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (error)
 				return error;
-			else {
-				inode->i_ctime = ext4_current_time(inode);
-				ext4_mark_inode_dirty(handle, inode);
-				if (error == 0)
-					acl = NULL;
-			}
+			inode->i_ctime = ext4_current_time(inode);
+			ext4_mark_inode_dirty(handle, inode);
 		}
 		break;
 
diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index 4dcc9e28dc5c..31344247ce89 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -210,12 +210,10 @@ static int __f2fs_set_acl(struct inode *inode, int type,
 	case ACL_TYPE_ACCESS:
 		name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (error < 0)
+			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (error)
 				return error;
 			set_acl_inode(inode, inode->i_mode);
-			if (error == 0)
-				acl = NULL;
 		}
 		break;
 
diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index 363ba9e9d8d0..2524807ee070 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -92,17 +92,11 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	if (type == ACL_TYPE_ACCESS) {
 		umode_t mode = inode->i_mode;
 
-		error = posix_acl_equiv_mode(acl, &mode);
-		if (error < 0)
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (error)
 			return error;
-
-		if (error == 0)
-			acl = NULL;
-
-		if (mode != inode->i_mode) {
-			inode->i_mode = mode;
+		if (mode != inode->i_mode)
 			mark_inode_dirty(inode);
-		}
 	}
 
 	if (acl) {
diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
index ab7ea2506b4d..9b92058a1240 100644
--- a/fs/hfsplus/posix_acl.c
+++ b/fs/hfsplus/posix_acl.c
@@ -65,8 +65,8 @@ int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
 	case ACL_TYPE_ACCESS:
 		xattr_name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			err = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (err < 0)
+			err = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (err)
 				return err;
 		}
 		err = 0;
diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c
index bc2693d56298..2a0f2a1044c1 100644
--- a/fs/jffs2/acl.c
+++ b/fs/jffs2/acl.c
@@ -233,9 +233,10 @@ int jffs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	case ACL_TYPE_ACCESS:
 		xprefix = JFFS2_XPREFIX_ACL_ACCESS;
 		if (acl) {
-			umode_t mode = inode->i_mode;
-			rc = posix_acl_equiv_mode(acl, &mode);
-			if (rc < 0)
+			umode_t mode;
+
+			rc = posix_acl_update_mode(inode, &mode, &acl);
+			if (rc)
 				return rc;
 			if (inode->i_mode != mode) {
 				struct iattr attr;
@@ -247,8 +248,6 @@ int jffs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 				if (rc < 0)
 					return rc;
 			}
-			if (rc == 0)
-				acl = NULL;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
index 21fa92ba2c19..3a1e1554a4e3 100644
--- a/fs/jfs/acl.c
+++ b/fs/jfs/acl.c
@@ -78,13 +78,11 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
 	case ACL_TYPE_ACCESS:
 		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			rc = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (rc < 0)
+			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (rc)
 				return rc;
 			inode->i_ctime = CURRENT_TIME;
 			mark_inode_dirty(inode);
-			if (rc == 0)
-				acl = NULL;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
index 2162434728c0..164307b99405 100644
--- a/fs/ocfs2/acl.c
+++ b/fs/ocfs2/acl.c
@@ -241,13 +241,11 @@ int ocfs2_set_acl(handle_t *handle,
 	case ACL_TYPE_ACCESS:
 		name_index = OCFS2_XATTR_INDEX_POSIX_ACL_ACCESS;
 		if (acl) {
-			umode_t mode = inode->i_mode;
-			ret = posix_acl_equiv_mode(acl, &mode);
-			if (ret < 0)
-				return ret;
+			umode_t mode;
 
-			if (ret == 0)
-				acl = NULL;
+			ret = posix_acl_update_mode(inode, &mode, &acl);
+			if (ret)
+				return ret;
 
 			ret = ocfs2_acl_set_mode(inode, di_bh,
 						 handle, mode);
diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
index 28f2195cd798..7a3754488312 100644
--- a/fs/orangefs/acl.c
+++ b/fs/orangefs/acl.c
@@ -73,14 +73,11 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			umode_t mode = inode->i_mode;
-			/*
-			 * can we represent this with the traditional file
-			 * mode permission bits?
-			 */
-			error = posix_acl_equiv_mode(acl, &mode);
-			if (error < 0) {
-				gossip_err("%s: posix_acl_equiv_mode err: %d\n",
+			umode_t mode;
+
+			error = posix_acl_update_mode(inode, &mode, &acl);
+			if (error) {
+				gossip_err("%s: posix_acl_update_mode err: %d\n",
 					   __func__,
 					   error);
 				return error;
@@ -90,8 +87,6 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 				SetModeFlag(orangefs_inode);
 			inode->i_mode = mode;
 			mark_inode_dirty_sync(inode);
-			if (error == 0)
-				acl = NULL;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 59d47ab0791a..bfc3ec388322 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -626,6 +626,37 @@ no_mem:
 }
 EXPORT_SYMBOL_GPL(posix_acl_create);
 
+/**
+ * posix_acl_update_mode  -  update mode in set_acl
+ *
+ * Update the file mode when setting an ACL: compute the new file permission
+ * bits based on the ACL.  In addition, if the ACL is equivalent to the new
+ * file mode, set *acl to NULL to indicate that no ACL should be set.
+ *
+ * As with chmod, clear the setgit bit if the caller is not in the owning group
+ * or capable of CAP_FSETID (see inode_change_ok).
+ *
+ * Called from set_acl inode operations.
+ */
+int posix_acl_update_mode(struct inode *inode, umode_t *mode_p,
+			  struct posix_acl **acl)
+{
+	umode_t mode = inode->i_mode;
+	int error;
+
+	error = posix_acl_equiv_mode(*acl, &mode);
+	if (error < 0)
+		return error;
+	if (error == 0)
+		*acl = NULL;
+	if (!in_group_p(inode->i_gid) &&
+	    !capable_wrt_inode_uidgid(inode, CAP_FSETID))
+		mode &= ~S_ISGID;
+	*mode_p = mode;
+	return 0;
+}
+EXPORT_SYMBOL(posix_acl_update_mode);
+
 /*
  * Fix up the uids and gids in posix acl extended attributes in place.
  */
diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
index dbed42f755e0..27376681c640 100644
--- a/fs/reiserfs/xattr_acl.c
+++ b/fs/reiserfs/xattr_acl.c
@@ -242,13 +242,9 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (error < 0)
+			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (error)
 				return error;
-			else {
-				if (error == 0)
-					acl = NULL;
-			}
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index b6e527b8eccb..8a0dec89ca56 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -257,16 +257,11 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode = inode->i_mode;
-		error = posix_acl_equiv_mode(acl, &mode);
-
-		if (error <= 0) {
-			acl = NULL;
-
-			if (error < 0)
-				return error;
-		}
+		umode_t mode;
 
+		error = posix_acl_update_mode(inode, &mode, &acl);
+		if (error)
+			return error;
 		error = xfs_set_mode(inode, mode);
 		if (error)
 			return error;
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index d5d3d741f028..bf1046d0397b 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -93,6 +93,7 @@ extern int set_posix_acl(struct inode *, int, struct posix_acl *);
 extern int posix_acl_chmod(struct inode *, umode_t);
 extern int posix_acl_create(struct inode *, umode_t *, struct posix_acl **,
 		struct posix_acl **);
+extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
 
 extern int simple_set_acl(struct inode *, struct posix_acl *, int);
 extern int simple_acl_create(struct inode *, struct inode *);
-- 
2.6.6

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Christoph Hellwig]

On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote:
> When file permissions are modified via chmod(2) and the user is not in
> the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> permissions as well as the new ACL, but doesn't clear the setgid bit in
> a similar way; this allows to bypass the check in chmod(2).  Fix that.

Looks good:

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

(although I would have split it into a refactor patch and the actual
fix for clearing the suid bit)

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Jeff Layton]

On Mon, 2016-09-19 at 17:42 +0200, Jan Kara wrote:
> When file permissions are modified via chmod(2) and the user is not in
> the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> permissions as well as the new ACL, but doesn't clear the setgid bit in
> a similar way; this allows to bypass the check in chmod(2).  Fix that.
> 
> References: CVE-2016-7097
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/9p/acl.c               | 40 +++++++++++++++++-----------------------
>  fs/btrfs/acl.c            |  6 ++----
>  fs/ceph/acl.c             |  6 ++----
>  fs/ext2/acl.c             | 12 ++++--------
>  fs/ext4/acl.c             | 12 ++++--------
>  fs/f2fs/acl.c             |  6 ++----
>  fs/gfs2/acl.c             | 12 +++---------
>  fs/hfsplus/posix_acl.c    |  4 ++--
>  fs/jffs2/acl.c            |  9 ++++-----
>  fs/jfs/acl.c              |  6 ++----
>  fs/ocfs2/acl.c            | 10 ++++------
>  fs/orangefs/acl.c         | 15 +++++----------
>  fs/posix_acl.c            | 31 +++++++++++++++++++++++++++++++
>  fs/reiserfs/xattr_acl.c   |  8 ++------
>  fs/xfs/xfs_acl.c          | 13 ++++---------
>  include/linux/posix_acl.h |  1 +
>  16 files changed, 89 insertions(+), 102 deletions(-)
> 
> Another forgotten patch. It was posted on May 27 and Aug 19. Al, can you
> please pick it up? Andrew, if Al does not respond, can you please take care
> of pushing it to Linus? Thanks!
> 
> diff --git a/fs/9p/acl.c b/fs/9p/acl.c
> index 5b6a1743ea17..b3c2cc79c20d 100644
> --- a/fs/9p/acl.c
> +++ b/fs/9p/acl.c
> @@ -276,32 +276,26 @@ static int v9fs_xattr_set_acl(const struct xattr_handler *handler,
> >  	switch (handler->flags) {
> >  	case ACL_TYPE_ACCESS:
> >  		if (acl) {
> > -			umode_t mode = inode->i_mode;
> > -			retval = posix_acl_equiv_mode(acl, &mode);
> > -			if (retval < 0)
> > +			struct iattr iattr;
> +
> > +			retval = posix_acl_update_mode(inode, &iattr.ia_mode, &acl);
> > +			if (retval)
> >  				goto err_out;
> > -			else {
> > -				struct iattr iattr;
> > -				if (retval == 0) {
> > -					/*
> > -					 * ACL can be represented
> > -					 * by the mode bits. So don't
> > -					 * update ACL.
> > -					 */
> > -					acl = NULL;
> > -					value = NULL;
> > -					size = 0;
> > -				}
> > -				/* Updte the mode bits */
> > -				iattr.ia_mode = ((mode & S_IALLUGO) |
> > -						 (inode->i_mode & ~S_IALLUGO));
> > -				iattr.ia_valid = ATTR_MODE;
> > -				/* FIXME should we update ctime ?
> > -				 * What is the following setxattr update the
> > -				 * mode ?
> > +			if (!acl) {
> > +				/*
> > +				 * ACL can be represented
> > +				 * by the mode bits. So don't
> > +				 * update ACL.
> >  				 */
> > -				v9fs_vfs_setattr_dotl(dentry, &iattr);
> > +				value = NULL;
> > +				size = 0;
> >  			}
> > +			iattr.ia_valid = ATTR_MODE;
> > +			/* FIXME should we update ctime ?
> > +			 * What is the following setxattr update the
> > +			 * mode ?
> > +			 */
> > +			v9fs_vfs_setattr_dotl(dentry, &iattr);
> >  		}
> >  		break;
> >  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 53bb7af4e5f0..247b8dfaf6e5 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -79,11 +79,9 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
> >  	case ACL_TYPE_ACCESS:
> >  		name = XATTR_NAME_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			ret = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -			if (ret < 0)
> > +			ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +			if (ret)
> >  				return ret;
> > -			if (ret == 0)
> > -				acl = NULL;
> >  		}
> >  		ret = 0;
> >  		break;
> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> index 4f67227f69a5..d0b6b342dff9 100644
> --- a/fs/ceph/acl.c
> +++ b/fs/ceph/acl.c
> @@ -95,11 +95,9 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  	case ACL_TYPE_ACCESS:
> >  		name = XATTR_NAME_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			ret = posix_acl_equiv_mode(acl, &new_mode);
> > -			if (ret < 0)
> > +			ret = posix_acl_update_mode(inode, &new_mode, &acl);
> > +			if (ret)
> >  				goto out;
> > -			if (ret == 0)
> > -				acl = NULL;
> >  		}
> >  		break;
> >  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index 42f1d1814083..e725aa0890e0 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -190,15 +190,11 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  		case ACL_TYPE_ACCESS:
> >  			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
> >  			if (acl) {
> > -				error = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -				if (error < 0)
> > +				error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +				if (error)
> >  					return error;
> > -				else {
> > -					inode->i_ctime = CURRENT_TIME_SEC;
> > -					mark_inode_dirty(inode);
> > -					if (error == 0)
> > -						acl = NULL;
> > -				}
> > +				inode->i_ctime = CURRENT_TIME_SEC;
> > +				mark_inode_dirty(inode);
> >  			}
> >  			break;
>  
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index c6601a476c02..dfa519979038 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -193,15 +193,11 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
> >  	case ACL_TYPE_ACCESS:
> >  		name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			error = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -			if (error < 0)
> > +			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +			if (error)
> >  				return error;
> > -			else {
> > -				inode->i_ctime = ext4_current_time(inode);
> > -				ext4_mark_inode_dirty(handle, inode);
> > -				if (error == 0)
> > -					acl = NULL;
> > -			}
> > +			inode->i_ctime = ext4_current_time(inode);
> > +			ext4_mark_inode_dirty(handle, inode);
> >  		}
> >  		break;
>  
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 4dcc9e28dc5c..31344247ce89 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -210,12 +210,10 @@ static int __f2fs_set_acl(struct inode *inode, int type,
> >  	case ACL_TYPE_ACCESS:
> >  		name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			error = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -			if (error < 0)
> > +			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +			if (error)
> >  				return error;
> >  			set_acl_inode(inode, inode->i_mode);
> > -			if (error == 0)
> > -				acl = NULL;
> >  		}
> >  		break;
>  
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 363ba9e9d8d0..2524807ee070 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -92,17 +92,11 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  	if (type == ACL_TYPE_ACCESS) {
> >  		umode_t mode = inode->i_mode;
>  
> > -		error = posix_acl_equiv_mode(acl, &mode);
> > -		if (error < 0)
> > +		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +		if (error)
> >  			return error;
> -
> > -		if (error == 0)
> > -			acl = NULL;
> -
> > -		if (mode != inode->i_mode) {
> > -			inode->i_mode = mode;
> > +		if (mode != inode->i_mode)
> >  			mark_inode_dirty(inode);
> > -		}
> >  	}
>  
> >  	if (acl) {
> diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
> index ab7ea2506b4d..9b92058a1240 100644
> --- a/fs/hfsplus/posix_acl.c
> +++ b/fs/hfsplus/posix_acl.c
> @@ -65,8 +65,8 @@ int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
> >  	case ACL_TYPE_ACCESS:
> >  		xattr_name = XATTR_NAME_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			err = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -			if (err < 0)
> > +			err = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +			if (err)
> >  				return err;
> >  		}
> >  		err = 0;
> diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c
> index bc2693d56298..2a0f2a1044c1 100644
> --- a/fs/jffs2/acl.c
> +++ b/fs/jffs2/acl.c
> @@ -233,9 +233,10 @@ int jffs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  	case ACL_TYPE_ACCESS:
> >  		xprefix = JFFS2_XPREFIX_ACL_ACCESS;
> >  		if (acl) {
> > -			umode_t mode = inode->i_mode;
> > -			rc = posix_acl_equiv_mode(acl, &mode);
> > -			if (rc < 0)
> > +			umode_t mode;
> +
> > +			rc = posix_acl_update_mode(inode, &mode, &acl);
> > +			if (rc)
> >  				return rc;
> >  			if (inode->i_mode != mode) {
> >  				struct iattr attr;
> @@ -247,8 +248,6 @@ int jffs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  				if (rc < 0)
> >  					return rc;
> >  			}
> > -			if (rc == 0)
> > -				acl = NULL;
> >  		}
> >  		break;
> >  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
> index 21fa92ba2c19..3a1e1554a4e3 100644
> --- a/fs/jfs/acl.c
> +++ b/fs/jfs/acl.c
> @@ -78,13 +78,11 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
> >  	case ACL_TYPE_ACCESS:
> >  		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			rc = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -			if (rc < 0)
> > +			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +			if (rc)
> >  				return rc;
> >  			inode->i_ctime = CURRENT_TIME;
> >  			mark_inode_dirty(inode);
> > -			if (rc == 0)
> > -				acl = NULL;
> >  		}
> >  		break;
> >  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
> index 2162434728c0..164307b99405 100644
> --- a/fs/ocfs2/acl.c
> +++ b/fs/ocfs2/acl.c
> @@ -241,13 +241,11 @@ int ocfs2_set_acl(handle_t *handle,
> >  	case ACL_TYPE_ACCESS:
> >  		name_index = OCFS2_XATTR_INDEX_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			umode_t mode = inode->i_mode;
> > -			ret = posix_acl_equiv_mode(acl, &mode);
> > -			if (ret < 0)
> > -				return ret;
> > +			umode_t mode;
>  
> > -			if (ret == 0)
> > -				acl = NULL;
> > +			ret = posix_acl_update_mode(inode, &mode, &acl);
> > +			if (ret)
> > +				return ret;
>  
> >  			ret = ocfs2_acl_set_mode(inode, di_bh,
> >  						 handle, mode);
> diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
> index 28f2195cd798..7a3754488312 100644
> --- a/fs/orangefs/acl.c
> +++ b/fs/orangefs/acl.c
> @@ -73,14 +73,11 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  	case ACL_TYPE_ACCESS:
> >  		name = XATTR_NAME_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			umode_t mode = inode->i_mode;
> > -			/*
> > -			 * can we represent this with the traditional file
> > -			 * mode permission bits?
> > -			 */
> > -			error = posix_acl_equiv_mode(acl, &mode);
> > -			if (error < 0) {
> > -				gossip_err("%s: posix_acl_equiv_mode err: %d\n",
> > +			umode_t mode;
> +
> > +			error = posix_acl_update_mode(inode, &mode, &acl);
> > +			if (error) {
> > +				gossip_err("%s: posix_acl_update_mode err: %d\n",
> >  					   __func__,
> >  					   error);
> >  				return error;
> @@ -90,8 +87,6 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  				SetModeFlag(orangefs_inode);
> >  			inode->i_mode = mode;
> >  			mark_inode_dirty_sync(inode);
> > -			if (error == 0)
> > -				acl = NULL;
> >  		}
> >  		break;
> >  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 59d47ab0791a..bfc3ec388322 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -626,6 +626,37 @@ no_mem:
>  }
>  EXPORT_SYMBOL_GPL(posix_acl_create);
>  
> +/**
> + * posix_acl_update_mode  -  update mode in set_acl
> + *
> + * Update the file mode when setting an ACL: compute the new file permission
> + * bits based on the ACL.  In addition, if the ACL is equivalent to the new
> + * file mode, set *acl to NULL to indicate that no ACL should be set.
> + *
> + * As with chmod, clear the setgit bit if the caller is not in the owning group
> + * or capable of CAP_FSETID (see inode_change_ok).
> + *
> + * Called from set_acl inode operations.
> + */
> +int posix_acl_update_mode(struct inode *inode, umode_t *mode_p,
> > +			  struct posix_acl **acl)
> +{
> > +	umode_t mode = inode->i_mode;
> > +	int error;
> +
> > +	error = posix_acl_equiv_mode(*acl, &mode);
> > +	if (error < 0)
> > +		return error;
> > +	if (error == 0)
> > +		*acl = NULL;
> > +	if (!in_group_p(inode->i_gid) &&
> > +	    !capable_wrt_inode_uidgid(inode, CAP_FSETID))
> > +		mode &= ~S_ISGID;
> > +	*mode_p = mode;
> > +	return 0;
> +}
> +EXPORT_SYMBOL(posix_acl_update_mode);
> +
>  /*
>   * Fix up the uids and gids in posix acl extended attributes in place.
>   */
> diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
> index dbed42f755e0..27376681c640 100644
> --- a/fs/reiserfs/xattr_acl.c
> +++ b/fs/reiserfs/xattr_acl.c
> @@ -242,13 +242,9 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
> >  	case ACL_TYPE_ACCESS:
> >  		name = XATTR_NAME_POSIX_ACL_ACCESS;
> >  		if (acl) {
> > -			error = posix_acl_equiv_mode(acl, &inode->i_mode);
> > -			if (error < 0)
> > +			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > +			if (error)
> >  				return error;
> > -			else {
> > -				if (error == 0)
> > -					acl = NULL;
> > -			}
> >  		}
> >  		break;
> >  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index b6e527b8eccb..8a0dec89ca56 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -257,16 +257,11 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  		return error;
>  
> >  	if (type == ACL_TYPE_ACCESS) {
> > -		umode_t mode = inode->i_mode;
> > -		error = posix_acl_equiv_mode(acl, &mode);
> -
> > -		if (error <= 0) {
> > -			acl = NULL;
> -
> > -			if (error < 0)
> > -				return error;
> > -		}
> > +		umode_t mode;
>  
> > +		error = posix_acl_update_mode(inode, &mode, &acl);
> > +		if (error)
> > +			return error;
> >  		error = xfs_set_mode(inode, mode);
> >  		if (error)
> >  			return error;
> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> index d5d3d741f028..bf1046d0397b 100644
> --- a/include/linux/posix_acl.h
> +++ b/include/linux/posix_acl.h
> @@ -93,6 +93,7 @@ extern int set_posix_acl(struct inode *, int, struct posix_acl *);
>  extern int posix_acl_chmod(struct inode *, umode_t);
>  extern int posix_acl_create(struct inode *, umode_t *, struct posix_acl **,
> >  		struct posix_acl **);
> +extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
>  
>  extern int simple_set_acl(struct inode *, struct posix_acl *, int);
>  extern int simple_acl_create(struct inode *, struct inode *);

Looks correct:

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

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Jan Kara]

On Tue 20-09-16 09:13:24, Christoph Hellwig wrote:
> On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote:
> > When file permissions are modified via chmod(2) and the user is not in
> > the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> > inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> > permissions as well as the new ACL, but doesn't clear the setgid bit in
> > a similar way; this allows to bypass the check in chmod(2).  Fix that.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for review.

> (although I would have split it into a refactor patch and the actual
> fix for clearing the suid bit)

Yeah, it would have been better but I don't think it would help to redo the
patch at this point.

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

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Eric Biggers]

On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote:
> When file permissions are modified via chmod(2) and the user is not in
> the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> permissions as well as the new ACL, but doesn't clear the setgid bit in
> a similar way; this allows to bypass the check in chmod(2).  Fix that.
> 

Hi Jan,

This patch is causing xfstests generic/314 to fail.  This test is supposed to
test "SGID inheritance on subdirectories", and the failure is because subdir2
unexpectedly ends up without a SGID bit.  This happens because the following
commands now result in the SGID bit on the parent directory "$TEST_DIR/$seq-dir"
being cleared rather than set:

	mkdir $TEST_DIR/$seq-dir
	chown $qa_user:12345 $TEST_DIR/$seq-dir
	chmod 2775 $TEST_DIR/$seq-dir
	su $qa_user -c "setfacl -m u:$qa_user:rwx,d:u:$qa_user:rwx $TEST_DIR/$seq-dir"

Is this the expected behavior now?

Thanks,

Eric

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Jan Kara]

Hi,

On Tue 11-10-16 16:11:01, Eric Biggers wrote:
> On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote:
> > When file permissions are modified via chmod(2) and the user is not in
> > the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> > inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> > permissions as well as the new ACL, but doesn't clear the setgid bit in
> > a similar way; this allows to bypass the check in chmod(2).  Fix that.
> > 
> 
> This patch is causing xfstests generic/314 to fail.  This test is supposed to
> test "SGID inheritance on subdirectories", and the failure is because subdir2
> unexpectedly ends up without a SGID bit.  This happens because the following
> commands now result in the SGID bit on the parent directory "$TEST_DIR/$seq-dir"
> being cleared rather than set:
> 
> 	mkdir $TEST_DIR/$seq-dir
> 	chown $qa_user:12345 $TEST_DIR/$seq-dir
> 	chmod 2775 $TEST_DIR/$seq-dir
> 	su $qa_user -c "setfacl -m u:$qa_user:rwx,d:u:$qa_user:rwx $TEST_DIR/$seq-dir"
> 
> Is this the expected behavior now?

Yes, this is expected behavior - $qa_user is not in group 12345 and thus he
could not set sgid bit himself. So once mode is modified by the user (and
the setfacl command you presented will touch file mode) sgid bit is expected
to be cleared - this is to be consistent with the behavior when:

  chmod 2755 $TEST_DIR/$seq-dir

done by $qa_user would clear the sgid bit as well.

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

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Eric Biggers]

Cc +fstests@vger.kernel.org

On Wed, Oct 12, 2016 at 09:16:51AM +0200, Jan Kara wrote:
> Hi,
> 
> On Tue 11-10-16 16:11:01, Eric Biggers wrote:
> > On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote:
> > > When file permissions are modified via chmod(2) and the user is not in
> > > the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> > > inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> > > permissions as well as the new ACL, but doesn't clear the setgid bit in
> > > a similar way; this allows to bypass the check in chmod(2).  Fix that.
> > > 
> > 
> > This patch is causing xfstests generic/314 to fail.  This test is supposed to
> > test "SGID inheritance on subdirectories", and the failure is because subdir2
> > unexpectedly ends up without a SGID bit.  This happens because the following
> > commands now result in the SGID bit on the parent directory "$TEST_DIR/$seq-dir"
> > being cleared rather than set:
> > 
> > 	mkdir $TEST_DIR/$seq-dir
> > 	chown $qa_user:12345 $TEST_DIR/$seq-dir
> > 	chmod 2775 $TEST_DIR/$seq-dir
> > 	su $qa_user -c "setfacl -m u:$qa_user:rwx,d:u:$qa_user:rwx $TEST_DIR/$seq-dir"
> > 
> > Is this the expected behavior now?
> 
> Yes, this is expected behavior - $qa_user is not in group 12345 and thus he
> could not set sgid bit himself. So once mode is modified by the user (and
> the setfacl command you presented will touch file mode) sgid bit is expected
> to be cleared - this is to be consistent with the behavior when:
> 
>   chmod 2755 $TEST_DIR/$seq-dir
> 
> done by $qa_user would clear the sgid bit as well.
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Is this true even though we're talking about a directory, not a regular file?

>From the POSIX man page for chmod (man 3 chmod):

       If the calling process does not have appropriate privileges, and if the
       group ID of the file does not match the effective group ID or  one  of
       the  supplementary  group  IDs  and  if the file is a regular file, bit
       S_ISGID (set-group-ID on execution) in the file's mode shall be cleared
       upon successful return from chmod().

Note the "is a regular file".  Granted, Linux already cleared the SGID bit on
directories too, so your patch is consistent with that existing behavior.  But
if "directories too" is really the "correct" behavior, it looks like there need
to be xfstests updates:

	* generic/314, which is supposed to test SGID inheritance, should be
	  updated to not depend on any specific SGID clearing behavior.  As-is
	  this test is failing.

	* generic/375, which is a new test that is supposed to test
	  SGID clearing, possibly should test both a regular file and a
	  directory.  Right now it only tests a regular file.

Thoughts on this?

Eric

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Dave Chinner]

On Wed, Oct 12, 2016 at 10:35:49AM -0700, Eric Biggers wrote:
> Cc +fstests@vger.kernel.org
> 
> On Wed, Oct 12, 2016 at 09:16:51AM +0200, Jan Kara wrote:
> > Hi,
> > 
> > On Tue 11-10-16 16:11:01, Eric Biggers wrote:
> > > On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote:
> > > > When file permissions are modified via chmod(2) and the user is not in
> > > > the owning group or capable of CAP_FSETID, the setgid bit is cleared in
> > > > inode_change_ok().  Setting a POSIX ACL via setxattr(2) sets the file
> > > > permissions as well as the new ACL, but doesn't clear the setgid bit in
> > > > a similar way; this allows to bypass the check in chmod(2).  Fix that.
> > > > 
> > > 
> > > This patch is causing xfstests generic/314 to fail.  This test is supposed to
> > > test "SGID inheritance on subdirectories", and the failure is because subdir2
> > > unexpectedly ends up without a SGID bit.  This happens because the following
> > > commands now result in the SGID bit on the parent directory "$TEST_DIR/$seq-dir"
> > > being cleared rather than set:
> > > 
> > > 	mkdir $TEST_DIR/$seq-dir
> > > 	chown $qa_user:12345 $TEST_DIR/$seq-dir
> > > 	chmod 2775 $TEST_DIR/$seq-dir
> > > 	su $qa_user -c "setfacl -m u:$qa_user:rwx,d:u:$qa_user:rwx $TEST_DIR/$seq-dir"
> > > 
> > > Is this the expected behavior now?
> > 
> > Yes, this is expected behavior - $qa_user is not in group 12345 and thus he
> > could not set sgid bit himself. So once mode is modified by the user (and
> > the setfacl command you presented will touch file mode) sgid bit is expected
> > to be cleared - this is to be consistent with the behavior when:
> > 
> >   chmod 2755 $TEST_DIR/$seq-dir
> > 
> > done by $qa_user would clear the sgid bit as well.
> > 
> > 								Honza
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> 
> Is this true even though we're talking about a directory, not a regular file?
> 
> From the POSIX man page for chmod (man 3 chmod):
> 
>        If the calling process does not have appropriate privileges, and if the
>        group ID of the file does not match the effective group ID or  one  of
>        the  supplementary  group  IDs  and  if the file is a regular file, bit
>        S_ISGID (set-group-ID on execution) in the file's mode shall be cleared
>        upon successful return from chmod().
> 
> Note the "is a regular file".  Granted, Linux already cleared the SGID bit on
> directories too, so your patch is consistent with that existing behavior.  But
> if "directories too" is really the "correct" behavior, it looks like there need
> to be xfstests updates:
> 
> 	* generic/314, which is supposed to test SGID inheritance, should be
> 	  updated to not depend on any specific SGID clearing behavior.  As-is
> 	  this test is failing.
> 
> 	* generic/375, which is a new test that is supposed to test
> 	  SGID clearing, possibly should test both a regular file and a
> 	  directory.  Right now it only tests a regular file.
> 
> Thoughts on this?

Send patches to fix/update the tests.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Eric Biggers]

On Thu, Oct 13, 2016 at 08:16:30AM +1100, Dave Chinner wrote:
> 
> Send patches to fix/update the tests.
> 

I will do so.

Eric