[PATCH 0/5] Failure to set acl may alter group permissions

[PATCH 0/5] Failure to set acl may alter group permissions

[Ernesto A. Fernández]

When changing a file's acl mask, the function that sets the access control
list (ext2_set_acl(), __ext4_set_acl(), __jfs_set_acl()...) will first set
the group permission bits of the file to the value of the mask (by calling
posix_acl_update_mode()), and only then set the actual extended attribute
representing the new acl.

The problem is, none of these functions try to restore the original
permission bits if the second part fails. If this happens to a file that
had no acl attributes to begin with, the system will from now on assume
that the mask permission bits are actual group permission bits,
potentially granting access to the wrong users.

If your working directory is on a filesystem mounted with extended user
attributes (and acl of course), this script will trigger the issue by
filling the drive:


touch test.file
chmod go-rwx test.file
yes xxxxxxxxxx > test.file
i=1
while setfattr -n user.$i test.file; do
  ((++i))
done
setfacl -m m:r test.file


By the time the script returns, the group that owns test.file may have read
permissions that were never granted. This happens reliably on at least
ext2, ext4, jfs and reiserfs.

I will follow this mail with patch drafts for those filesystems. I believe
most filesystems that support acl will need a patch, but perhaps it's best
if I share what I have so far.

Thank you for your attention.

Ernesto A. Fern√°ndez (5):
  ext4: preserve i_mode if __ext4_set_acl() fails
  ext2: preserve i_mode if ext2_set_acl() fails
  ext2: fix line over 80 characters in ext2_set_acl()
  jfs: preserve i_mode if __jfs_set_acl() fails
  reiserfs: preserve i_mode if __reiserfs_set_acl() fails

 fs/ext2/acl.c           | 25 ++++++++++++++++---------
 fs/ext4/acl.c           | 15 +++++++++++----
 fs/jfs/acl.c            | 15 +++++++++++----
 fs/reiserfs/xattr.c     |  4 ++++
 fs/reiserfs/xattr_acl.c | 25 +++++++++----------------
 5 files changed, 51 insertions(+), 33 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ext4: preserve i_mode if __ext4_set_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, __ext4_set_acl() will first set the group
bits of i_mode to the value of the mask, and only then set the actual
extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file
had no acl attribute to begin with, the system will from now on assume
that the mask permission bits are actual group permission bits, potentially
granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fern√°ndez <ernesto.mnd.fernandez@gmail.com>
---
 fs/ext4/acl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 09441ae..2985cd0 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -189,16 +189,17 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			error = posix_acl_update_mode(inode, &mode, &acl);
 			if (error)
 				return error;
-			inode->i_ctime = current_time(inode);
-			ext4_mark_inode_dirty(handle, inode);
+			update_mode = 1;
 		}
 		break;
 
@@ -221,8 +222,14 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 				      value, size, xattr_flags);
 
 	kfree(value);
-	if (!error)
+	if (!error) {
 		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
+			inode->i_ctime = current_time(inode);
+			ext4_mark_inode_dirty(handle, inode);
+		}
+	}
 
 	return error;
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] ext4: preserve i_mode if __ext4_set_acl() fails

[Jan Kara]

On Wed 12-07-17 06:53:51, Ernesto A. Fernández wrote:
> When changing a file's acl mask, __ext4_set_acl() will first set the group
> bits of i_mode to the value of the mask, and only then set the actual
> extended attribute representing the new acl.
> 
> If the second part fails (due to lack of space, for example) and the file
> had no acl attribute to begin with, the system will from now on assume
> that the mask permission bits are actual group permission bits, potentially
> granting access to the wrong users.
> 
> Prevent this by only changing the inode mode after the acl has been set.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

This clashes with my fix of another bug in that area [1] but the fixup
should be straightforward. The patch looks good to me so feel free to add

Reviewed-by: Jan Kara <jack@suse.cz>

[1] https://www.spinics.net/lists/stable/msg178111.html

								Honza

> ---
>  fs/ext4/acl.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index 09441ae..2985cd0 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -189,16 +189,17 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
>  	void *value = NULL;
>  	size_t size = 0;
>  	int error;
> +	int update_mode = 0;
> +	umode_t mode = inode->i_mode;
>  
>  	switch (type) {
>  	case ACL_TYPE_ACCESS:
>  		name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
>  		if (acl) {
> -			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +			error = posix_acl_update_mode(inode, &mode, &acl);
>  			if (error)
>  				return error;
> -			inode->i_ctime = current_time(inode);
> -			ext4_mark_inode_dirty(handle, inode);
> +			update_mode = 1;
>  		}
>  		break;
>  
> @@ -221,8 +222,14 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
>  				      value, size, xattr_flags);
>  
>  	kfree(value);
> -	if (!error)
> +	if (!error) {
>  		set_cached_acl(inode, type, acl);
> +		if (update_mode) {
> +			inode->i_mode = mode;
> +			inode->i_ctime = current_time(inode);
> +			ext4_mark_inode_dirty(handle, inode);
> +		}
> +	}
>  
>  	return error;
>  }
> -- 
> 2.1.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/5] ext4: preserve i_mode if __ext4_set_acl() fails

[Theodore Ts'o]

On Wed, Jul 12, 2017 at 06:53:51AM -0300, Ernesto A. Fernández wrote:
> When changing a file's acl mask, __ext4_set_acl() will first set the group
> bits of i_mode to the value of the mask, and only then set the actual
> extended attribute representing the new acl.
> 
> If the second part fails (due to lack of space, for example) and the file
> had no acl attribute to begin with, the system will from now on assume
> that the mask permission bits are actual group permission bits, potentially
> granting access to the wrong users.
> 
> Prevent this by only changing the inode mode after the acl has been set.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

Thanks, applied.

						- Ted

[PATCH 2/5] ext2: preserve i_mode if ext2_set_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, ext2_set_acl() will first set the group
bits of i_mode to the value of the mask, and only then set the actual
extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file
had no acl attribute to begin with, the system will from now on assume
that the mask permission bits are actual group permission bits, potentially
granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fern√°ndez <ernesto.mnd.fernandez@gmail.com>
---
A line in acl.c is too long, and checkpatch.pl complains about it when run
against this patch. In case it matters, the next patch will fix that.

 fs/ext2/acl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
index 79dafa7..4e04b7e 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -185,16 +185,17 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch(type) {
 		case ACL_TYPE_ACCESS:
 			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
 			if (acl) {
-				error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+				error = posix_acl_update_mode(inode, &mode, &acl);
 				if (error)
 					return error;
-				inode->i_ctime = current_time(inode);
-				mark_inode_dirty(inode);
+				update_mode = 1;
 			}
 			break;
 
@@ -216,8 +217,14 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	error = ext2_xattr_set(inode, name_index, "", value, size, 0);
 
 	kfree(value);
-	if (!error)
+	if (!error) {
 		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
+			inode->i_ctime = current_time(inode);
+			mark_inode_dirty(inode);
+		}
+	}
 	return error;
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] ext2: preserve i_mode if ext2_set_acl() fails

[Jan Kara]

On Wed 12-07-17 06:54:19, Ernesto A. Fernández wrote:
> When changing a file's acl mask, ext2_set_acl() will first set the group
> bits of i_mode to the value of the mask, and only then set the actual
> extended attribute representing the new acl.
> 
> If the second part fails (due to lack of space, for example) and the file
> had no acl attribute to begin with, the system will from now on assume
> that the mask permission bits are actual group permission bits, potentially
> granting access to the wrong users.
> 
> Prevent this by only changing the inode mode after the acl has been set.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

Thanks for the patch! I have rebased it on top of the fix in that area that
is sitting in my tree and merged it to my tree.

								Honza

> ---
> A line in acl.c is too long, and checkpatch.pl complains about it when run
> against this patch. In case it matters, the next patch will fix that.
> 
>  fs/ext2/acl.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index 79dafa7..4e04b7e 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -185,16 +185,17 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	void *value = NULL;
>  	size_t size = 0;
>  	int error;
> +	int update_mode = 0;
> +	umode_t mode = inode->i_mode;
>  
>  	switch(type) {
>  		case ACL_TYPE_ACCESS:
>  			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
>  			if (acl) {
> -				error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +				error = posix_acl_update_mode(inode, &mode, &acl);
>  				if (error)
>  					return error;
> -				inode->i_ctime = current_time(inode);
> -				mark_inode_dirty(inode);
> +				update_mode = 1;
>  			}
>  			break;
>  
> @@ -216,8 +217,14 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	error = ext2_xattr_set(inode, name_index, "", value, size, 0);
>  
>  	kfree(value);
> -	if (!error)
> +	if (!error) {
>  		set_cached_acl(inode, type, acl);
> +		if (update_mode) {
> +			inode->i_mode = mode;
> +			inode->i_ctime = current_time(inode);
> +			mark_inode_dirty(inode);
> +		}
> +	}
>  	return error;
>  }
>  
> -- 
> 2.1.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/5] ext2: fix line over 80 characters in ext2_set_acl()

[Ernesto A. Fernández]

Shorten the name of the error variable to 'err' so the call to
posix_acl_update_mode() fits within 80 characters.

Signed-off-by: Ernesto A. Fern√°ndez <ernesto.mnd.fernandez@gmail.com>
---
 fs/ext2/acl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
index 4e04b7e..d2cca5c 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -184,7 +184,7 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	int name_index;
 	void *value = NULL;
 	size_t size = 0;
-	int error;
+	int err;
 	int update_mode = 0;
 	umode_t mode = inode->i_mode;
 
@@ -192,9 +192,9 @@ 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_update_mode(inode, &mode, &acl);
-				if (error)
-					return error;
+				err = posix_acl_update_mode(inode, &mode, &acl);
+				if (err)
+					return err;
 				update_mode = 1;
 			}
 			break;
@@ -214,10 +214,10 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			return (int)PTR_ERR(value);
 	}
 
-	error = ext2_xattr_set(inode, name_index, "", value, size, 0);
+	err = ext2_xattr_set(inode, name_index, "", value, size, 0);
 
 	kfree(value);
-	if (!error) {
+	if (!err) {
 		set_cached_acl(inode, type, acl);
 		if (update_mode) {
 			inode->i_mode = mode;
@@ -225,7 +225,7 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			mark_inode_dirty(inode);
 		}
 	}
-	return error;
+	return err;
 }
 
 /*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] ext2: fix line over 80 characters in ext2_set_acl()

[Jan Kara]

On Wed 12-07-17 06:54:50, Ernesto A. Fernández wrote:
> Shorten the name of the error variable to 'err' so the call to
> posix_acl_update_mode() fits within 80 characters.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

This is no longer a problem. Patch ignored.

								Honza

> ---
>  fs/ext2/acl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index 4e04b7e..d2cca5c 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -184,7 +184,7 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	int name_index;
>  	void *value = NULL;
>  	size_t size = 0;
> -	int error;
> +	int err;
>  	int update_mode = 0;
>  	umode_t mode = inode->i_mode;
>  
> @@ -192,9 +192,9 @@ 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_update_mode(inode, &mode, &acl);
> -				if (error)
> -					return error;
> +				err = posix_acl_update_mode(inode, &mode, &acl);
> +				if (err)
> +					return err;
>  				update_mode = 1;
>  			}
>  			break;
> @@ -214,10 +214,10 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  			return (int)PTR_ERR(value);
>  	}
>  
> -	error = ext2_xattr_set(inode, name_index, "", value, size, 0);
> +	err = ext2_xattr_set(inode, name_index, "", value, size, 0);
>  
>  	kfree(value);
> -	if (!error) {
> +	if (!err) {
>  		set_cached_acl(inode, type, acl);
>  		if (update_mode) {
>  			inode->i_mode = mode;
> @@ -225,7 +225,7 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  			mark_inode_dirty(inode);
>  		}
>  	}
> -	return error;
> +	return err;
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/5] jfs: preserve i_mode if __jfs_set_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, __jfs_set_acl() will first set the group
bits of i_mode to the value of the mask, and only then set the actual
extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file
had no acl attribute to begin with, the system will from now on assume
that the mask permission bits are actual group permission bits, potentially
granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fern√°ndez <ernesto.mnd.fernandez@gmail.com>
---
 fs/jfs/acl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
index 7bc186f..db5a385 100644
--- a/fs/jfs/acl.c
+++ b/fs/jfs/acl.c
@@ -73,16 +73,17 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
 	int rc;
 	int size = 0;
 	char *value = NULL;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			rc = posix_acl_update_mode(inode, &mode, &acl);
 			if (rc)
 				return rc;
-			inode->i_ctime = current_time(inode);
-			mark_inode_dirty(inode);
+			update_mode = 1;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
@@ -105,8 +106,14 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
 out:
 	kfree(value);
 
-	if (!rc)
+	if (!rc) {
 		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
+			inode->i_ctime = current_time(inode);
+			mark_inode_dirty(inode);
+		}
+	}
 
 	return rc;
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] jfs: preserve i_mode if __jfs_set_acl() fails

[Dave Kleikamp]

On 07/12/2017 04:55 AM, Ernesto A. Fernández wrote:
> When changing a file's acl mask, __jfs_set_acl() will first set the group
> bits of i_mode to the value of the mask, and only then set the actual
> extended attribute representing the new acl.
> 
> If the second part fails (due to lack of space, for example) and the file
> had no acl attribute to begin with, the system will from now on assume
> that the mask permission bits are actual group permission bits, potentially
> granting access to the wrong users.
> 
> Prevent this by only changing the inode mode after the acl has been set.

This looks good to me. I'll add it to the jfs queue.

Thanks,
Shaggy

> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> ---
>  fs/jfs/acl.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
> index 7bc186f..db5a385 100644
> --- a/fs/jfs/acl.c
> +++ b/fs/jfs/acl.c
> @@ -73,16 +73,17 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
>  	int rc;
>  	int size = 0;
>  	char *value = NULL;
> +	int update_mode = 0;
> +	umode_t mode = inode->i_mode;
>  
>  	switch (type) {
>  	case ACL_TYPE_ACCESS:
>  		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
>  		if (acl) {
> -			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +			rc = posix_acl_update_mode(inode, &mode, &acl);
>  			if (rc)
>  				return rc;
> -			inode->i_ctime = current_time(inode);
> -			mark_inode_dirty(inode);
> +			update_mode = 1;
>  		}
>  		break;
>  	case ACL_TYPE_DEFAULT:
> @@ -105,8 +106,14 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
>  out:
>  	kfree(value);
>  
> -	if (!rc)
> +	if (!rc) {
>  		set_cached_acl(inode, type, acl);
> +		if (update_mode) {
> +			inode->i_mode = mode;
> +			inode->i_ctime = current_time(inode);
> +			mark_inode_dirty(inode);
> +		}
> +	}
>  
>  	return rc;
>  }
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Jfs-discussion mailing list
Jfs-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jfs-discussion

[PATCH 5/5] reiserfs: preserve i_mode if __reiserfs_set_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, __reiserfs_set_acl() will first set the
group bits of i_mode to the value of the mask, and only then set the
actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file
had no acl attribute to begin with, the system will from now on assume
that the mask permission bits are actual group permission bits, potentially
granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.
Also make reiserfs_xattr_set_handle() return -ENODATA only in case of
actual error, and return 0 when requested deletion of a nonexistent acl.
This is more consistent with the behaviour of other *_xattr_set()
functions and makes the patch simpler.

Signed-off-by: Ernesto A. Fern√°ndez <ernesto.mnd.fernandez@gmail.com>
---
 fs/reiserfs/xattr.c     |  4 ++++
 fs/reiserfs/xattr_acl.c | 25 +++++++++----------------
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index e87aa21..936a57a 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -513,6 +513,10 @@ reiserfs_xattr_set_handle(struct reiserfs_transaction_handle *th,
 
 	if (!buffer) {
 		err = lookup_and_delete_xattr(inode, name);
+		if (flags & XATTR_REPLACE)
+			return err;
+		if (err == -ENODATA)
+			err = 0;
 		return err;
 	}
 
diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
index 3d2256a..c02e5ac 100644
--- a/fs/reiserfs/xattr_acl.c
+++ b/fs/reiserfs/xattr_acl.c
@@ -237,14 +237,17 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			error = posix_acl_update_mode(inode, &mode, &acl);
 			if (error)
 				return error;
+			update_mode = 1;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
@@ -264,25 +267,15 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 
 	error = reiserfs_xattr_set_handle(th, inode, name, value, size, 0);
 
-	/*
-	 * Ensure that the inode gets dirtied if we're only using
-	 * the mode bits and an old ACL didn't exist. We don't need
-	 * to check if the inode is hashed here since we won't get
-	 * called by reiserfs_inherit_default_acl().
-	 */
-	if (error == -ENODATA) {
-		error = 0;
-		if (type == ACL_TYPE_ACCESS) {
+	kfree(value);
+	if (!error) {
+		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
 			inode->i_ctime = current_time(inode);
 			mark_inode_dirty(inode);
 		}
 	}
-
-	kfree(value);
-
-	if (!error)
-		set_cached_acl(inode, type, acl);

Re: [PATCH 5/5] reiserfs: preserve i_mode if __reiserfs_set_acl() fails

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Wed 12-07-17 06:56:04, Ernesto A. Fernández wrote:
> When changing a file's acl mask, __reiserfs_set_acl() will first set the
> group bits of i_mode to the value of the mask, and only then set the
> actual extended attribute representing the new acl.
> 
> If the second part fails (due to lack of space, for example) and the file
> had no acl attribute to begin with, the system will from now on assume
> that the mask permission bits are actual group permission bits, potentially
> granting access to the wrong users.
> 
> Prevent this by only changing the inode mode after the acl has been set.
> Also make reiserfs_xattr_set_handle() return -ENODATA only in case of
> actual error, and return 0 when requested deletion of a nonexistent acl.
> This is more consistent with the behaviour of other *_xattr_set()
> functions and makes the patch simpler.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

Thanks for the patch!  I have rebased this onto my tree and merged the
resulting patch (attached).

								Honza

> ---
>  fs/reiserfs/xattr.c     |  4 ++++
>  fs/reiserfs/xattr_acl.c | 25 +++++++++----------------
>  2 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
> index e87aa21..936a57a 100644
> --- a/fs/reiserfs/xattr.c
> +++ b/fs/reiserfs/xattr.c
> @@ -513,6 +513,10 @@ reiserfs_xattr_set_handle(struct reiserfs_transaction_handle *th,
>  
>  	if (!buffer) {
>  		err = lookup_and_delete_xattr(inode, name);
> +		if (flags & XATTR_REPLACE)
> +			return err;
> +		if (err == -ENODATA)
> +			err = 0;
>  		return err;
>  	}
>  
> diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
> index 3d2256a..c02e5ac 100644
> --- a/fs/reiserfs/xattr_acl.c
> +++ b/fs/reiserfs/xattr_acl.c
> @@ -237,14 +237,17 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
>  	void *value = NULL;
>  	size_t size = 0;
>  	int error;
> +	int update_mode = 0;
> +	umode_t mode = inode->i_mode;
>  
>  	switch (type) {
>  	case ACL_TYPE_ACCESS:
>  		name = XATTR_NAME_POSIX_ACL_ACCESS;
>  		if (acl) {
> -			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +			error = posix_acl_update_mode(inode, &mode, &acl);
>  			if (error)
>  				return error;
> +			update_mode = 1;
>  		}
>  		break;
>  	case ACL_TYPE_DEFAULT:
> @@ -264,25 +267,15 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
>  
>  	error = reiserfs_xattr_set_handle(th, inode, name, value, size, 0);
>  
> -	/*
> -	 * Ensure that the inode gets dirtied if we're only using
> -	 * the mode bits and an old ACL didn't exist. We don't need
> -	 * to check if the inode is hashed here since we won't get
> -	 * called by reiserfs_inherit_default_acl().
> -	 */
> -	if (error == -ENODATA) {
> -		error = 0;
> -		if (type == ACL_TYPE_ACCESS) {
> +	kfree(value);
> +	if (!error) {
> +		set_cached_acl(inode, type, acl);
> +		if (update_mode) {
> +			inode->i_mode = mode;
>  			inode->i_ctime = current_time(inode);
>  			mark_inode_dirty(inode);
>  		}
>  	}
> -
> -	kfree(value);
> -
> -	if (!error)
> -		set_cached_acl(inode, type, acl);
> -
>  	return error;
>  }
>  
> -- 
> 2.1.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-reiserfs-preserve-i_mode-if-__reiserfs_set_acl-fails.patch --]
[-- Type: text/x-patch, Size: 1494 bytes --]

From 71edef97f294dbff5d84bca5721c7b30273a0786 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ernesto=20A=2E=20Fern=C3=A1ndez?=
 <ernesto.mnd.fernandez@gmail.com>
Date: Mon, 17 Jul 2017 18:42:41 +0200
Subject: [PATCH] reiserfs: preserve i_mode if __reiserfs_set_acl() fails
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When changing a file's acl mask, reiserfs_set_acl() will first set the
group bits of i_mode to the value of the mask, and only then set the
actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the
file had no acl attribute to begin with, the system will from now on
assume that the mask permission bits are actual group permission bits,
potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/reiserfs/xattr_acl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
index d92a1dc6ee70..54415f0e3d18 100644
--- a/fs/reiserfs/xattr_acl.c
+++ b/fs/reiserfs/xattr_acl.c
@@ -23,7 +23,8 @@ reiserfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	struct reiserfs_transaction_handle th;
 	size_t jcreate_blocks;
 	int size = acl ? posix_acl_xattr_size(acl->a_count) : 0;

Re: [PATCH 0/5] Failure to set acl may alter group permissions

[Christoph Hellwig]

On Wed, Jul 12, 2017 at 06:53:22AM -0300, Ernesto A. Fernández wrote:
> If your working directory is on a filesystem mounted with extended user
> attributes (and acl of course), this script will trigger the issue by
> filling the drive:
> 
> 
> touch test.file
> chmod go-rwx test.file
> yes xxxxxxxxxx > test.file
> i=1
> while setfattr -n user.$i test.file; do
>   ((++i))
> done
> setfacl -m m:r test.file

Can you please wire this up for xfstests?

[xfstests PATCH] generic: add test of file mode when setfacl fails

[Ernesto A. Fernández]

Check that the group permission bits of a file are not altered when setfacl
fails. At the time of this patch the test fails for at least ext2, ext4 and
jfs. It is not run against reiserfs, since xfstests claims that "attrs are
not supported by this filesystem type".

The failure to set acls is induced by filling the device, so to save time
this should probably be run with a small TEST_DEV.

Note that this test is not meaningful for all filesystems, because some
will still succeed in setting the acls. This does not mean they don't have
a bug in how they would handle an error.

Signed-off-by: Ernesto A. Fern√°ndez <ernesto.mnd.fernandez@gmail.com>
---
 tests/generic/447     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/447.out |  2 ++
 tests/generic/group   |  1 +
 3 files changed, 91 insertions(+)
 create mode 100755 tests/generic/447
 create mode 100644 tests/generic/447.out

diff --git a/tests/generic/447 b/tests/generic/447
new file mode 100755
index 0000000..b5fabd3
--- /dev/null
+++ b/tests/generic/447
@@ -0,0 +1,88 @@
+#! /bin/bash
+# FS QA Test 447
+#
+# Fill the device and set as many extended attributes to a file as
+# possible. Then call setfacl on it and, if this fails for lack of
+# space, test that the permissions remain the same.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Ernesto A. Fernandez.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd $TEST_DIR
+	rm -f $TFILE
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_test
+_require_acls
+_require_attrs
+
+cd $TEST_DIR
+TFILE=testfile.$seq
+
+# Create the test file and choose its permissions
+rm -f $TFILE
+touch $TFILE
+chmod u+rwx $TFILE
+chmod go-rwx $TFILE
+
+# Try to run out of space so setfacl will fail
+yes xxxxxxxxxx &> $TFILE
+i=1
+while setfattr -n user.$i $TFILE &> /dev/null; do
+	((++i))
+done
+
+if setfacl -m m:r $TFILE &> /dev/null; then
+	# setfacl succeeded, so the test was meaningless
+	# The filesystem might still have an issue
+	status=0
+	echo "-rwx------"
+	exit
+fi
+
+# Since setfacl failed, the permissions should not have changed
+stat -c %A $TFILE
+
+status=0
+exit
diff --git a/tests/generic/447.out b/tests/generic/447.out
new file mode 100644
index 0000000..adec877
--- /dev/null
+++ b/tests/generic/447.out
@@ -0,0 +1,2 @@
+QA output created by 447
+-rwx------
diff --git a/tests/generic/group b/tests/generic/group
index 8c1e21a..f11d798 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -449,3 +449,4 @@
 444 auto quick acl
 445 auto quick rw
 446 auto quick rw dangerous
+447 acl
-- 
2.1.4


--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [xfstests PATCH] generic: add test of file mode when setfacl fails

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 06:40:04AM -0300, Ernesto A. Fernández wrote:
> Check that the group permission bits of a file are not altered when setfacl
> fails. At the time of this patch the test fails for at least ext2, ext4 and
> jfs. It is not run against reiserfs, since xfstests claims that "attrs are
> not supported by this filesystem type".

That's odd, as common/config explicitly turns on user xattrs:

        reiserfs)
                # acls & xattrs aren't turned on by default on reiserfs
                export MOUNT_OPTIONS="-o acl,user_xattr $REISERFS_MOUNT_OPTIONS"


> 
> The failure to set acls is induced by filling the device, so to save time
> this should probably be run with a small TEST_DEV.

If you run it on the scratch fs instead of the test fs you can use
_scratch_mkfs_sized to hand craft a small fs, which would be nice
to get a decent runtime.

Re: [xfstests PATCH] generic: add test of file mode when setfacl fails

[Ernesto A. Fernández]

On Thu, Jul 13, 2017 at 04:55:07AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 13, 2017 at 06:40:04AM -0300, Ernesto A. Fernández wrote:
> > Check that the group permission bits of a file are not altered when setfacl
> > fails. At the time of this patch the test fails for at least ext2, ext4 and
> > jfs. It is not run against reiserfs, since xfstests claims that "attrs are
> > not supported by this filesystem type".
> 
> That's odd, as common/config explicitly turns on user xattrs:
> 

xfstests decides if a filesystem supports attributes by trying to set them on
the root inode, but reiserfs does not allow this. If this is actually the
intended behaviour of reiserfs then xfstests should use a different check, but
I'm guessing it's a bug, perhaps in mkfs.reiserfs.

> 
> > 
> > The failure to set acls is induced by filling the device, so to save time
> > this should probably be run with a small TEST_DEV.
> 
> If you run it on the scratch fs instead of the test fs you can use
> _scratch_mkfs_sized to hand craft a small fs, which would be nice
> to get a decent runtime.

That's much better, thank you. I will send a revision soon. For some reason
_scratch_mkfs_sized did not have support for jfs, but it was trivial to add.