diff mbox series

[trusty,CVE-2016-7097,1/1] posix_acl: Clear SGID bit when setting file permissions

Message ID 20170906085453.22382-2-juerg.haefliger@canonical.com
State New
Headers show
Series Fix for CVE-2016-7097 | expand

Commit Message

Juerg Haefliger Sept. 6, 2017, 8:54 a.m. UTC
From: Jan Kara <jack@suse.cz>

commit 073931017b49d9458aa351605b43a7e34598caef upstream.

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
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
[bwh: Backported to 3.16:
 - Drop changes to orangefs
 - Adjust context
 - Update ext3 as well]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

CVE-2016-7097

[juergh: Backported to 3.13:
 - Drop changes to ceph
 - Use capable() instead of capable_wrt_inode_uidgid()
 - Update generic_acl.c as well
 - In gfs2, jfs, and xfs, take care to avoid leaking the allocated ACL if
   posix_acl_update_mode() determines it's not needed]
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
---
 fs/9p/acl.c               | 40 +++++++++++++++++-----------------------
 fs/btrfs/acl.c            |  6 ++----
 fs/ext2/acl.c             | 12 ++++--------
 fs/ext3/acl.c             | 12 ++++--------
 fs/ext4/acl.c             | 12 ++++--------
 fs/f2fs/acl.c             |  6 ++----
 fs/generic_acl.c          | 15 ++++++++-------
 fs/gfs2/acl.c             | 16 +++++++---------
 fs/hfsplus/posix_acl.c    |  4 ++--
 fs/jffs2/acl.c            |  9 ++++-----
 fs/jfs/xattr.c            |  6 ++++--
 fs/ocfs2/acl.c            |  9 +++------
 fs/posix_acl.c            | 30 ++++++++++++++++++++++++++++++
 fs/reiserfs/xattr_acl.c   |  8 ++------
 fs/xfs/xfs_acl.c          | 17 +++++++----------
 include/linux/posix_acl.h |  1 +
 16 files changed, 101 insertions(+), 102 deletions(-)

Comments

Kleber Sacilotto de Souza Sept. 6, 2017, 12:59 p.m. UTC | #1
On 09/06/17 10:54, Juerg Haefliger wrote:
> From: Jan Kara <jack@suse.cz>
> 
> commit 073931017b49d9458aa351605b43a7e34598caef upstream.
> 
> 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
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> [bwh: Backported to 3.16:
>  - Drop changes to orangefs
>  - Adjust context
>  - Update ext3 as well]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> CVE-2016-7097
> 

Should we add here the sha1 of the 3.16 backport commit since the
original SOB comes from it?

Probably:
(backported from f2ba3e2310b3967720b83126db8684c69ce41894 3.16.y)

I think we can add that while applying the patch.

Otherwise it looks a sane backport and a nice combination of the patches
from 3.2 and 3.16 :-).


Kleber

> [juergh: Backported to 3.13:
>  - Drop changes to ceph
>  - Use capable() instead of capable_wrt_inode_uidgid()
>  - Update generic_acl.c as well
>  - In gfs2, jfs, and xfs, take care to avoid leaking the allocated ACL if
>    posix_acl_update_mode() determines it's not needed]
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> ---
>  fs/9p/acl.c               | 40 +++++++++++++++++-----------------------
>  fs/btrfs/acl.c            |  6 ++----
>  fs/ext2/acl.c             | 12 ++++--------
>  fs/ext3/acl.c             | 12 ++++--------
>  fs/ext4/acl.c             | 12 ++++--------
>  fs/f2fs/acl.c             |  6 ++----
>  fs/generic_acl.c          | 15 ++++++++-------
>  fs/gfs2/acl.c             | 16 +++++++---------
>  fs/hfsplus/posix_acl.c    |  4 ++--
>  fs/jffs2/acl.c            |  9 ++++-----
>  fs/jfs/xattr.c            |  6 ++++--
>  fs/ocfs2/acl.c            |  9 +++------
>  fs/posix_acl.c            | 30 ++++++++++++++++++++++++++++++
>  fs/reiserfs/xattr_acl.c   |  8 ++------
>  fs/xfs/xfs_acl.c          | 17 +++++++----------
>  include/linux/posix_acl.h |  1 +
>  16 files changed, 101 insertions(+), 102 deletions(-)
> 
> diff --git a/fs/9p/acl.c b/fs/9p/acl.c
> index 7af425f53bee..9686c1f17653 100644
> --- a/fs/9p/acl.c
> +++ b/fs/9p/acl.c
> @@ -320,32 +320,26 @@ static int v9fs_xattr_set_acl(struct dentry *dentry, const char *name,
>  	case ACL_TYPE_ACCESS:
>  		name = POSIX_ACL_XATTR_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 0890c83643e9..d6d53e5e7945 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -118,11 +118,9 @@ static int btrfs_set_acl(struct btrfs_trans_handle *trans,
>  	case ACL_TYPE_ACCESS:
>  		name = POSIX_ACL_XATTR_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/ext2/acl.c b/fs/ext2/acl.c
> index 110b6b371a4e..48c3c2d7d261 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -206,15 +206,11 @@ ext2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>  		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/ext3/acl.c b/fs/ext3/acl.c
> index dbb5ad59a7fc..bb2f60a62d82 100644
> --- a/fs/ext3/acl.c
> +++ b/fs/ext3/acl.c
> @@ -205,15 +205,11 @@ ext3_set_acl(handle_t *handle, struct inode *inode, int type,
>  		case ACL_TYPE_ACCESS:
>  			name_index = EXT3_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;
> -					ext3_mark_inode_dirty(handle, inode);
> -					if (error == 0)
> -						acl = NULL;
> -				}
> +				inode->i_ctime = CURRENT_TIME_SEC;
> +				ext3_mark_inode_dirty(handle, inode);
>  			}
>  			break;
>  
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index 39a54a0e9fe4..c844f1bfb451 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -211,15 +211,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 d0fc287efeff..0eb2d66827ad 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -224,12 +224,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(fi, inode->i_mode);
> -			if (error == 0)
> -				acl = NULL;
>  		}
>  		break;
>  
> diff --git a/fs/generic_acl.c b/fs/generic_acl.c
> index b3f3676796d3..67319f168b42 100644
> --- a/fs/generic_acl.c
> +++ b/fs/generic_acl.c
> @@ -86,16 +86,17 @@ generic_acl_set(struct dentry *dentry, const char *name, const void *value,
>  		if (error)
>  			goto failed;
>  		switch (type) {
> -		case ACL_TYPE_ACCESS:
> -			error = posix_acl_equiv_mode(acl, &inode->i_mode);
> -			if (error < 0)
> +		case ACL_TYPE_ACCESS: {
> +			struct posix_acl *saved_acl = acl;
> +
> +			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +			if (acl == NULL)
> +				posix_acl_release(saved_acl);
> +			if (error)
>  				goto failed;
>  			inode->i_ctime = CURRENT_TIME;
> -			if (error == 0) {
> -				posix_acl_release(acl);
> -				acl = NULL;
> -			}
>  			break;
> +		}
>  		case ACL_TYPE_DEFAULT:
>  			if (!S_ISDIR(inode->i_mode)) {
>  				error = -EINVAL;
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index f69ac0af5496..015809a066b5 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -267,16 +267,14 @@ static int gfs2_xattr_system_set(struct dentry *dentry, const char *name,
>  		goto out_release;
>  
>  	if (type == ACL_TYPE_ACCESS) {
> -		umode_t mode = inode->i_mode;
> -		error = posix_acl_equiv_mode(acl, &mode);
> +		struct posix_acl *saved_acl = acl;
> +		umode_t mode;
>  
> -		if (error <= 0) {
> -			posix_acl_release(acl);
> -			acl = NULL;
> -
> -			if (error < 0)
> -				return error;
> -		}
> +		error = posix_acl_update_mode(inode, &mode, &acl);
> +		if (error || acl == NULL)
> +			posix_acl_release(saved_acl);
> +		if (error)
> +			return error;
>  
>  		error = gfs2_set_mode(inode, mode);
>  		if (error)
> diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
> index b609cc14c72e..9f7cc491ffb1 100644
> --- a/fs/hfsplus/posix_acl.c
> +++ b/fs/hfsplus/posix_acl.c
> @@ -72,8 +72,8 @@ static int hfsplus_set_posix_acl(struct inode *inode,
>  	case ACL_TYPE_ACCESS:
>  		xattr_name = POSIX_ACL_XATTR_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 223283c30111..9335b8d3cf52 100644
> --- a/fs/jffs2/acl.c
> +++ b/fs/jffs2/acl.c
> @@ -243,9 +243,10 @@ static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>  	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;
> @@ -257,8 +258,6 @@ static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>  				if (rc < 0)
>  					return rc;
>  			}
> -			if (rc == 0)
> -				acl = NULL;
>  		}
>  		break;
>  	case ACL_TYPE_DEFAULT:
> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
> index d3472f4cd530..6910662a8bf5 100644
> --- a/fs/jfs/xattr.c
> +++ b/fs/jfs/xattr.c
> @@ -693,9 +693,11 @@ static int can_set_system_xattr(struct inode *inode, const char *name,
>  			return rc;
>  		}
>  		if (acl) {
> -			rc = posix_acl_equiv_mode(acl, &inode->i_mode);
> +			struct posix_acl *dummy = acl;
> +
> +			rc = posix_acl_update_mode(inode, &inode->i_mode, &dummy);
>  			posix_acl_release(acl);
> -			if (rc < 0) {
> +			if (rc) {
>  				printk(KERN_ERR
>  				       "posix_acl_equiv_mode returned %d\n",
>  				       rc);
> diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
> index b4f788e0ca31..b16bb5c70bc8 100644
> --- a/fs/ocfs2/acl.c
> +++ b/fs/ocfs2/acl.c
> @@ -270,14 +270,11 @@ static 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)
> +			umode_t mode;
> +			ret = posix_acl_update_mode(inode, &mode, &acl);
> +			if (ret)
>  				return ret;
>  			else {
> -				if (ret == 0)
> -					acl = NULL;
> -
>  				ret = ocfs2_acl_set_mode(inode, di_bh,
>  							 handle, mode);
>  				if (ret)
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 3542f1f814e2..8161e5c9dc31 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -407,6 +407,36 @@ posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p)
>  }
>  EXPORT_SYMBOL(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(CAP_FSETID))
> +		mode &= ~S_ISGID;
> +	*mode_p = mode;
> +	return 0;
> +}
> +EXPORT_SYMBOL(posix_acl_update_mode);
> +
>  int
>  posix_acl_chmod(struct posix_acl **acl, gfp_t gfp, umode_t mode)
>  {
> diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
> index 06c04f73da65..a86ad7ec7957 100644
> --- a/fs/reiserfs/xattr_acl.c
> +++ b/fs/reiserfs/xattr_acl.c
> @@ -288,13 +288,9 @@ reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
>  	case ACL_TYPE_ACCESS:
>  		name = POSIX_ACL_XATTR_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 370eb3e121d1..89ac0522b38d 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -402,17 +402,14 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
>  		goto out_release;
>  
>  	if (type == ACL_TYPE_ACCESS) {
> -		umode_t mode = inode->i_mode;
> -		error = posix_acl_equiv_mode(acl, &mode);
> -
> -		if (error <= 0) {
> -			posix_acl_release(acl);
> -			acl = NULL;
> -
> -			if (error < 0)
> -				return error;
> -		}
> +		struct posix_acl *saved_acl = acl;
> +		umode_t mode;
>  
> +		error = posix_acl_update_mode(inode, &mode, &acl);
> +		if (error || acl == NULL)
> +			posix_acl_release(saved_acl);
> +		if (error)
> +			return error;
>  		error = xfs_set_mode(inode, mode);
>  		if (error)
>  			goto out_release;
> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> index 7931efe71175..2ae0bba45f12 100644
> --- a/include/linux/posix_acl.h
> +++ b/include/linux/posix_acl.h
> @@ -90,6 +90,7 @@ extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
>  extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
>  extern int posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
>  extern int posix_acl_chmod(struct posix_acl **, gfp_t, umode_t);
> +extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
>  
>  extern struct posix_acl *get_posix_acl(struct inode *, int);
>  extern int set_posix_acl(struct inode *, int, struct posix_acl *);
>
Kleber Sacilotto de Souza Sept. 6, 2017, 1 p.m. UTC | #2
On 09/06/17 14:59, Kleber Souza wrote:
> On 09/06/17 10:54, Juerg Haefliger wrote:
>> From: Jan Kara <jack@suse.cz>
>>
>> commit 073931017b49d9458aa351605b43a7e34598caef upstream.
>>
>> 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
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>> Signed-off-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>> [bwh: Backported to 3.16:
>>  - Drop changes to orangefs
>>  - Adjust context
>>  - Update ext3 as well]
>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
>>
>> CVE-2016-7097
>>
> 
> Should we add here the sha1 of the 3.16 backport commit since the
> original SOB comes from it?
> 
> Probably:
> (backported from f2ba3e2310b3967720b83126db8684c69ce41894 3.16.y)
> 
> I think we can add that while applying the patch.
> 
> Otherwise it looks a sane backport and a nice combination of the patches
> from 3.2 and 3.16 :-).

Now with the actual ACK:

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> 
> 
> Kleber
> 
>> [juergh: Backported to 3.13:
>>  - Drop changes to ceph
>>  - Use capable() instead of capable_wrt_inode_uidgid()
>>  - Update generic_acl.c as well
>>  - In gfs2, jfs, and xfs, take care to avoid leaking the allocated ACL if
>>    posix_acl_update_mode() determines it's not needed]
>> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
>> ---
>>  fs/9p/acl.c               | 40 +++++++++++++++++-----------------------
>>  fs/btrfs/acl.c            |  6 ++----
>>  fs/ext2/acl.c             | 12 ++++--------
>>  fs/ext3/acl.c             | 12 ++++--------
>>  fs/ext4/acl.c             | 12 ++++--------
>>  fs/f2fs/acl.c             |  6 ++----
>>  fs/generic_acl.c          | 15 ++++++++-------
>>  fs/gfs2/acl.c             | 16 +++++++---------
>>  fs/hfsplus/posix_acl.c    |  4 ++--
>>  fs/jffs2/acl.c            |  9 ++++-----
>>  fs/jfs/xattr.c            |  6 ++++--
>>  fs/ocfs2/acl.c            |  9 +++------
>>  fs/posix_acl.c            | 30 ++++++++++++++++++++++++++++++
>>  fs/reiserfs/xattr_acl.c   |  8 ++------
>>  fs/xfs/xfs_acl.c          | 17 +++++++----------
>>  include/linux/posix_acl.h |  1 +
>>  16 files changed, 101 insertions(+), 102 deletions(-)
>>
>> diff --git a/fs/9p/acl.c b/fs/9p/acl.c
>> index 7af425f53bee..9686c1f17653 100644
>> --- a/fs/9p/acl.c
>> +++ b/fs/9p/acl.c
>> @@ -320,32 +320,26 @@ static int v9fs_xattr_set_acl(struct dentry *dentry, const char *name,
>>  	case ACL_TYPE_ACCESS:
>>  		name = POSIX_ACL_XATTR_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 0890c83643e9..d6d53e5e7945 100644
>> --- a/fs/btrfs/acl.c
>> +++ b/fs/btrfs/acl.c
>> @@ -118,11 +118,9 @@ static int btrfs_set_acl(struct btrfs_trans_handle *trans,
>>  	case ACL_TYPE_ACCESS:
>>  		name = POSIX_ACL_XATTR_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/ext2/acl.c b/fs/ext2/acl.c
>> index 110b6b371a4e..48c3c2d7d261 100644
>> --- a/fs/ext2/acl.c
>> +++ b/fs/ext2/acl.c
>> @@ -206,15 +206,11 @@ ext2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>>  		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/ext3/acl.c b/fs/ext3/acl.c
>> index dbb5ad59a7fc..bb2f60a62d82 100644
>> --- a/fs/ext3/acl.c
>> +++ b/fs/ext3/acl.c
>> @@ -205,15 +205,11 @@ ext3_set_acl(handle_t *handle, struct inode *inode, int type,
>>  		case ACL_TYPE_ACCESS:
>>  			name_index = EXT3_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;
>> -					ext3_mark_inode_dirty(handle, inode);
>> -					if (error == 0)
>> -						acl = NULL;
>> -				}
>> +				inode->i_ctime = CURRENT_TIME_SEC;
>> +				ext3_mark_inode_dirty(handle, inode);
>>  			}
>>  			break;
>>  
>> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
>> index 39a54a0e9fe4..c844f1bfb451 100644
>> --- a/fs/ext4/acl.c
>> +++ b/fs/ext4/acl.c
>> @@ -211,15 +211,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 d0fc287efeff..0eb2d66827ad 100644
>> --- a/fs/f2fs/acl.c
>> +++ b/fs/f2fs/acl.c
>> @@ -224,12 +224,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(fi, inode->i_mode);
>> -			if (error == 0)
>> -				acl = NULL;
>>  		}
>>  		break;
>>  
>> diff --git a/fs/generic_acl.c b/fs/generic_acl.c
>> index b3f3676796d3..67319f168b42 100644
>> --- a/fs/generic_acl.c
>> +++ b/fs/generic_acl.c
>> @@ -86,16 +86,17 @@ generic_acl_set(struct dentry *dentry, const char *name, const void *value,
>>  		if (error)
>>  			goto failed;
>>  		switch (type) {
>> -		case ACL_TYPE_ACCESS:
>> -			error = posix_acl_equiv_mode(acl, &inode->i_mode);
>> -			if (error < 0)
>> +		case ACL_TYPE_ACCESS: {
>> +			struct posix_acl *saved_acl = acl;
>> +
>> +			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
>> +			if (acl == NULL)
>> +				posix_acl_release(saved_acl);
>> +			if (error)
>>  				goto failed;
>>  			inode->i_ctime = CURRENT_TIME;
>> -			if (error == 0) {
>> -				posix_acl_release(acl);
>> -				acl = NULL;
>> -			}
>>  			break;
>> +		}
>>  		case ACL_TYPE_DEFAULT:
>>  			if (!S_ISDIR(inode->i_mode)) {
>>  				error = -EINVAL;
>> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
>> index f69ac0af5496..015809a066b5 100644
>> --- a/fs/gfs2/acl.c
>> +++ b/fs/gfs2/acl.c
>> @@ -267,16 +267,14 @@ static int gfs2_xattr_system_set(struct dentry *dentry, const char *name,
>>  		goto out_release;
>>  
>>  	if (type == ACL_TYPE_ACCESS) {
>> -		umode_t mode = inode->i_mode;
>> -		error = posix_acl_equiv_mode(acl, &mode);
>> +		struct posix_acl *saved_acl = acl;
>> +		umode_t mode;
>>  
>> -		if (error <= 0) {
>> -			posix_acl_release(acl);
>> -			acl = NULL;
>> -
>> -			if (error < 0)
>> -				return error;
>> -		}
>> +		error = posix_acl_update_mode(inode, &mode, &acl);
>> +		if (error || acl == NULL)
>> +			posix_acl_release(saved_acl);
>> +		if (error)
>> +			return error;
>>  
>>  		error = gfs2_set_mode(inode, mode);
>>  		if (error)
>> diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
>> index b609cc14c72e..9f7cc491ffb1 100644
>> --- a/fs/hfsplus/posix_acl.c
>> +++ b/fs/hfsplus/posix_acl.c
>> @@ -72,8 +72,8 @@ static int hfsplus_set_posix_acl(struct inode *inode,
>>  	case ACL_TYPE_ACCESS:
>>  		xattr_name = POSIX_ACL_XATTR_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 223283c30111..9335b8d3cf52 100644
>> --- a/fs/jffs2/acl.c
>> +++ b/fs/jffs2/acl.c
>> @@ -243,9 +243,10 @@ static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>>  	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;
>> @@ -257,8 +258,6 @@ static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>>  				if (rc < 0)
>>  					return rc;
>>  			}
>> -			if (rc == 0)
>> -				acl = NULL;
>>  		}
>>  		break;
>>  	case ACL_TYPE_DEFAULT:
>> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
>> index d3472f4cd530..6910662a8bf5 100644
>> --- a/fs/jfs/xattr.c
>> +++ b/fs/jfs/xattr.c
>> @@ -693,9 +693,11 @@ static int can_set_system_xattr(struct inode *inode, const char *name,
>>  			return rc;
>>  		}
>>  		if (acl) {
>> -			rc = posix_acl_equiv_mode(acl, &inode->i_mode);
>> +			struct posix_acl *dummy = acl;
>> +
>> +			rc = posix_acl_update_mode(inode, &inode->i_mode, &dummy);
>>  			posix_acl_release(acl);
>> -			if (rc < 0) {
>> +			if (rc) {
>>  				printk(KERN_ERR
>>  				       "posix_acl_equiv_mode returned %d\n",
>>  				       rc);
>> diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
>> index b4f788e0ca31..b16bb5c70bc8 100644
>> --- a/fs/ocfs2/acl.c
>> +++ b/fs/ocfs2/acl.c
>> @@ -270,14 +270,11 @@ static 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)
>> +			umode_t mode;
>> +			ret = posix_acl_update_mode(inode, &mode, &acl);
>> +			if (ret)
>>  				return ret;
>>  			else {
>> -				if (ret == 0)
>> -					acl = NULL;
>> -
>>  				ret = ocfs2_acl_set_mode(inode, di_bh,
>>  							 handle, mode);
>>  				if (ret)
>> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
>> index 3542f1f814e2..8161e5c9dc31 100644
>> --- a/fs/posix_acl.c
>> +++ b/fs/posix_acl.c
>> @@ -407,6 +407,36 @@ posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p)
>>  }
>>  EXPORT_SYMBOL(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(CAP_FSETID))
>> +		mode &= ~S_ISGID;
>> +	*mode_p = mode;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(posix_acl_update_mode);
>> +
>>  int
>>  posix_acl_chmod(struct posix_acl **acl, gfp_t gfp, umode_t mode)
>>  {
>> diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
>> index 06c04f73da65..a86ad7ec7957 100644
>> --- a/fs/reiserfs/xattr_acl.c
>> +++ b/fs/reiserfs/xattr_acl.c
>> @@ -288,13 +288,9 @@ reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
>>  	case ACL_TYPE_ACCESS:
>>  		name = POSIX_ACL_XATTR_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 370eb3e121d1..89ac0522b38d 100644
>> --- a/fs/xfs/xfs_acl.c
>> +++ b/fs/xfs/xfs_acl.c
>> @@ -402,17 +402,14 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
>>  		goto out_release;
>>  
>>  	if (type == ACL_TYPE_ACCESS) {
>> -		umode_t mode = inode->i_mode;
>> -		error = posix_acl_equiv_mode(acl, &mode);
>> -
>> -		if (error <= 0) {
>> -			posix_acl_release(acl);
>> -			acl = NULL;
>> -
>> -			if (error < 0)
>> -				return error;
>> -		}
>> +		struct posix_acl *saved_acl = acl;
>> +		umode_t mode;
>>  
>> +		error = posix_acl_update_mode(inode, &mode, &acl);
>> +		if (error || acl == NULL)
>> +			posix_acl_release(saved_acl);
>> +		if (error)
>> +			return error;
>>  		error = xfs_set_mode(inode, mode);
>>  		if (error)
>>  			goto out_release;
>> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
>> index 7931efe71175..2ae0bba45f12 100644
>> --- a/include/linux/posix_acl.h
>> +++ b/include/linux/posix_acl.h
>> @@ -90,6 +90,7 @@ extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
>>  extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
>>  extern int posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
>>  extern int posix_acl_chmod(struct posix_acl **, gfp_t, umode_t);
>> +extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
>>  
>>  extern struct posix_acl *get_posix_acl(struct inode *, int);
>>  extern int set_posix_acl(struct inode *, int, struct posix_acl *);
>>
Thadeu Lima de Souza Cascardo Sept. 6, 2017, 1:40 p.m. UTC | #3
On Wed, Sep 06, 2017 at 10:54:53AM +0200, Juerg Haefliger wrote:
> From: Jan Kara <jack@suse.cz>
> 
> commit 073931017b49d9458aa351605b43a7e34598caef upstream.
> 
> 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
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> [bwh: Backported to 3.16:
>  - Drop changes to orangefs
>  - Adjust context
>  - Update ext3 as well]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> CVE-2016-7097
> 
> [juergh: Backported to 3.13:
>  - Drop changes to ceph
>  - Use capable() instead of capable_wrt_inode_uidgid()

We have capable_wrt_inode_uidgid in trusty. Why didn't you use it?

Cascardo.
Juerg Haefliger Sept. 6, 2017, 3:41 p.m. UTC | #4
On 09/06/2017 03:40 PM, Thadeu Lima de Souza Cascardo wrote:
> On Wed, Sep 06, 2017 at 10:54:53AM +0200, Juerg Haefliger wrote:
>> From: Jan Kara <jack@suse.cz>
>>
>> commit 073931017b49d9458aa351605b43a7e34598caef upstream.
>>
>> 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
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>> Signed-off-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>> [bwh: Backported to 3.16:
>>  - Drop changes to orangefs
>>  - Adjust context
>>  - Update ext3 as well]
>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
>>
>> CVE-2016-7097
>>
>> [juergh: Backported to 3.13:
>>  - Drop changes to ceph
>>  - Use capable() instead of capable_wrt_inode_uidgid()
> 
> We have capable_wrt_inode_uidgid in trusty. Why didn't you use it?

Because I was looking at upstream 3.13 and not trusty 3.13. Duh.

...Juerg

> Cascardo.
>
diff mbox series

Patch

diff --git a/fs/9p/acl.c b/fs/9p/acl.c
index 7af425f53bee..9686c1f17653 100644
--- a/fs/9p/acl.c
+++ b/fs/9p/acl.c
@@ -320,32 +320,26 @@  static int v9fs_xattr_set_acl(struct dentry *dentry, const char *name,
 	case ACL_TYPE_ACCESS:
 		name = POSIX_ACL_XATTR_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 0890c83643e9..d6d53e5e7945 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -118,11 +118,9 @@  static int btrfs_set_acl(struct btrfs_trans_handle *trans,
 	case ACL_TYPE_ACCESS:
 		name = POSIX_ACL_XATTR_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/ext2/acl.c b/fs/ext2/acl.c
index 110b6b371a4e..48c3c2d7d261 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -206,15 +206,11 @@  ext2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
 		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/ext3/acl.c b/fs/ext3/acl.c
index dbb5ad59a7fc..bb2f60a62d82 100644
--- a/fs/ext3/acl.c
+++ b/fs/ext3/acl.c
@@ -205,15 +205,11 @@  ext3_set_acl(handle_t *handle, struct inode *inode, int type,
 		case ACL_TYPE_ACCESS:
 			name_index = EXT3_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;
-					ext3_mark_inode_dirty(handle, inode);
-					if (error == 0)
-						acl = NULL;
-				}
+				inode->i_ctime = CURRENT_TIME_SEC;
+				ext3_mark_inode_dirty(handle, inode);
 			}
 			break;
 
diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 39a54a0e9fe4..c844f1bfb451 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -211,15 +211,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 d0fc287efeff..0eb2d66827ad 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -224,12 +224,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(fi, inode->i_mode);
-			if (error == 0)
-				acl = NULL;
 		}
 		break;
 
diff --git a/fs/generic_acl.c b/fs/generic_acl.c
index b3f3676796d3..67319f168b42 100644
--- a/fs/generic_acl.c
+++ b/fs/generic_acl.c
@@ -86,16 +86,17 @@  generic_acl_set(struct dentry *dentry, const char *name, const void *value,
 		if (error)
 			goto failed;
 		switch (type) {
-		case ACL_TYPE_ACCESS:
-			error = posix_acl_equiv_mode(acl, &inode->i_mode);
-			if (error < 0)
+		case ACL_TYPE_ACCESS: {
+			struct posix_acl *saved_acl = acl;
+
+			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			if (acl == NULL)
+				posix_acl_release(saved_acl);
+			if (error)
 				goto failed;
 			inode->i_ctime = CURRENT_TIME;
-			if (error == 0) {
-				posix_acl_release(acl);
-				acl = NULL;
-			}
 			break;
+		}
 		case ACL_TYPE_DEFAULT:
 			if (!S_ISDIR(inode->i_mode)) {
 				error = -EINVAL;
diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index f69ac0af5496..015809a066b5 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -267,16 +267,14 @@  static int gfs2_xattr_system_set(struct dentry *dentry, const char *name,
 		goto out_release;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode = inode->i_mode;
-		error = posix_acl_equiv_mode(acl, &mode);
+		struct posix_acl *saved_acl = acl;
+		umode_t mode;
 
-		if (error <= 0) {
-			posix_acl_release(acl);
-			acl = NULL;
-
-			if (error < 0)
-				return error;
-		}
+		error = posix_acl_update_mode(inode, &mode, &acl);
+		if (error || acl == NULL)
+			posix_acl_release(saved_acl);
+		if (error)
+			return error;
 
 		error = gfs2_set_mode(inode, mode);
 		if (error)
diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
index b609cc14c72e..9f7cc491ffb1 100644
--- a/fs/hfsplus/posix_acl.c
+++ b/fs/hfsplus/posix_acl.c
@@ -72,8 +72,8 @@  static int hfsplus_set_posix_acl(struct inode *inode,
 	case ACL_TYPE_ACCESS:
 		xattr_name = POSIX_ACL_XATTR_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 223283c30111..9335b8d3cf52 100644
--- a/fs/jffs2/acl.c
+++ b/fs/jffs2/acl.c
@@ -243,9 +243,10 @@  static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
 	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;
@@ -257,8 +258,6 @@  static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl)
 				if (rc < 0)
 					return rc;
 			}
-			if (rc == 0)
-				acl = NULL;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
index d3472f4cd530..6910662a8bf5 100644
--- a/fs/jfs/xattr.c
+++ b/fs/jfs/xattr.c
@@ -693,9 +693,11 @@  static int can_set_system_xattr(struct inode *inode, const char *name,
 			return rc;
 		}
 		if (acl) {
-			rc = posix_acl_equiv_mode(acl, &inode->i_mode);
+			struct posix_acl *dummy = acl;
+
+			rc = posix_acl_update_mode(inode, &inode->i_mode, &dummy);
 			posix_acl_release(acl);
-			if (rc < 0) {
+			if (rc) {
 				printk(KERN_ERR
 				       "posix_acl_equiv_mode returned %d\n",
 				       rc);
diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
index b4f788e0ca31..b16bb5c70bc8 100644
--- a/fs/ocfs2/acl.c
+++ b/fs/ocfs2/acl.c
@@ -270,14 +270,11 @@  static 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)
+			umode_t mode;
+			ret = posix_acl_update_mode(inode, &mode, &acl);
+			if (ret)
 				return ret;
 			else {
-				if (ret == 0)
-					acl = NULL;
-
 				ret = ocfs2_acl_set_mode(inode, di_bh,
 							 handle, mode);
 				if (ret)
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 3542f1f814e2..8161e5c9dc31 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -407,6 +407,36 @@  posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p)
 }
 EXPORT_SYMBOL(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(CAP_FSETID))
+		mode &= ~S_ISGID;
+	*mode_p = mode;
+	return 0;
+}
+EXPORT_SYMBOL(posix_acl_update_mode);
+
 int
 posix_acl_chmod(struct posix_acl **acl, gfp_t gfp, umode_t mode)
 {
diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
index 06c04f73da65..a86ad7ec7957 100644
--- a/fs/reiserfs/xattr_acl.c
+++ b/fs/reiserfs/xattr_acl.c
@@ -288,13 +288,9 @@  reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 	case ACL_TYPE_ACCESS:
 		name = POSIX_ACL_XATTR_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 370eb3e121d1..89ac0522b38d 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -402,17 +402,14 @@  xfs_xattr_acl_set(struct dentry *dentry, const char *name,
 		goto out_release;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode = inode->i_mode;
-		error = posix_acl_equiv_mode(acl, &mode);
-
-		if (error <= 0) {
-			posix_acl_release(acl);
-			acl = NULL;
-
-			if (error < 0)
-				return error;
-		}
+		struct posix_acl *saved_acl = acl;
+		umode_t mode;
 
+		error = posix_acl_update_mode(inode, &mode, &acl);
+		if (error || acl == NULL)
+			posix_acl_release(saved_acl);
+		if (error)
+			return error;
 		error = xfs_set_mode(inode, mode);
 		if (error)
 			goto out_release;
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 7931efe71175..2ae0bba45f12 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -90,6 +90,7 @@  extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
 extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
 extern int posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
 extern int posix_acl_chmod(struct posix_acl **, gfp_t, umode_t);
+extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
 
 extern struct posix_acl *get_posix_acl(struct inode *, int);
 extern int set_posix_acl(struct inode *, int, struct posix_acl *);