[PATCH v2] btrfs: Check if root is readonly while setting security xattr

[PATCH v2] btrfs: Check if root is readonly while setting security xattr

[Goldwyn Rodrigues]

For a filesystem which has btrfs read-only property set to true, all
write operations including xattr should be denied. However, security
xattr can still be changed even if btrfs ro property is true.

This happens because xattr_permission() does not have any restrictions
on security.*, system.*  and in some cases trusted.* from VFS and
the decision is left to the underlying filesystem. See comments in
xattr_permission() for more details.

This patch checks if the root is read-only before performing the set
xattr operation.

Testcase: 

#!/bin/bash

DEV=/dev/vdb
MNT=/mnt

mkfs.btrfs -f $DEV
mount $DEV $MNT
echo "file one" > $MNT/f1

setfattr -n "security.one" -v 2 $MNT/f1
btrfs property set /mnt ro true

# Following statement should fail
setfattr -n "security.one" -v 1 $MNT/f1

umount $MNT

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>


diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 7421abcf325a..5bb8d8c86311 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -371,6 +371,9 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
 				   const char *name, const void *buffer,
 				   size_t size, int flags)
 {
+	if (btrfs_root_readonly(BTRFS_I(inode)->root))
+		return -EROFS;
+
 	name = xattr_full_name(handler, name);
 	return btrfs_setxattr_trans(inode, name, buffer, size, flags);
 }

-- 
Goldwyn

Re: [PATCH v2] btrfs: Check if root is readonly while setting security xattr

[Filipe Manana]

On Tue, Aug 16, 2022 at 04:42:56PM -0500, Goldwyn Rodrigues wrote:
> For a filesystem which has btrfs read-only property set to true, all
> write operations including xattr should be denied. However, security
> xattr can still be changed even if btrfs ro property is true.
> 
> This happens because xattr_permission() does not have any restrictions
> on security.*, system.*  and in some cases trusted.* from VFS and
> the decision is left to the underlying filesystem. See comments in
> xattr_permission() for more details.
> 
> This patch checks if the root is read-only before performing the set
> xattr operation.
> 
> Testcase: 
> 
> #!/bin/bash
> 
> DEV=/dev/vdb
> MNT=/mnt
> 
> mkfs.btrfs -f $DEV
> mount $DEV $MNT
> echo "file one" > $MNT/f1
> 
> setfattr -n "security.one" -v 2 $MNT/f1
> btrfs property set /mnt ro true
> 
> # Following statement should fail
> setfattr -n "security.one" -v 1 $MNT/f1
> 
> umount $MNT
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good now, thanks.

Btw, as noted in the review of the test case for fstests, the subject
should be "btrfs: check ..." and not "btrfs: Check ...", as that's
the convention used for btrfs. David will likely change that when
picking the patch, as usual.

> 
> 
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 7421abcf325a..5bb8d8c86311 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -371,6 +371,9 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
>  				   const char *name, const void *buffer,
>  				   size_t size, int flags)
>  {
> +	if (btrfs_root_readonly(BTRFS_I(inode)->root))
> +		return -EROFS;
> +
>  	name = xattr_full_name(handler, name);
>  	return btrfs_setxattr_trans(inode, name, buffer, size, flags);
>  }
> 
> -- 
> Goldwyn

Re: [PATCH v2] btrfs: Check if root is readonly while setting security xattr

[David Sterba]

On Tue, Aug 16, 2022 at 04:42:56PM -0500, Goldwyn Rodrigues wrote:
> For a filesystem which has btrfs read-only property set to true, all
> write operations including xattr should be denied. However, security
> xattr can still be changed even if btrfs ro property is true.
> 
> This happens because xattr_permission() does not have any restrictions
> on security.*, system.*  and in some cases trusted.* from VFS and
> the decision is left to the underlying filesystem. See comments in
> xattr_permission() for more details.
> 
> This patch checks if the root is read-only before performing the set
> xattr operation.
> 
> Testcase: 
> 
> #!/bin/bash
> 
> DEV=/dev/vdb
> MNT=/mnt
> 
> mkfs.btrfs -f $DEV
> mount $DEV $MNT
> echo "file one" > $MNT/f1
> 
> setfattr -n "security.one" -v 2 $MNT/f1
> btrfs property set /mnt ro true
> 
> # Following statement should fail
> setfattr -n "security.one" -v 1 $MNT/f1
> 
> umount $MNT
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Added to misc-next, thanks.

Re: [PATCH v2] btrfs: Check if root is readonly while setting security xattr

[Anand Jain]

On 8/17/22 05:42, Goldwyn Rodrigues wrote:
> For a filesystem which has btrfs read-only property set to true, all
> write operations including xattr should be denied. However, security
> xattr can still be changed even if btrfs ro property is true.
> 
> This happens because xattr_permission() does not have any restrictions
> on security.*, system.*  and in some cases trusted.* from VFS and
> the decision is left to the underlying filesystem. See comments in
> xattr_permission() for more details.
> 

> This patch checks if the root is read-only before performing the set
> xattr operation.
> 

Now we match to the mount -o ro behaviour.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

> Testcase:
> 
> #!/bin/bash
> 
> DEV=/dev/vdb
> MNT=/mnt
> 
> mkfs.btrfs -f $DEV
> mount $DEV $MNT
> echo "file one" > $MNT/f1
> 
> setfattr -n "security.one" -v 2 $MNT/f1
> btrfs property set /mnt ro true
> 
> # Following statement should fail
> setfattr -n "security.one" -v 1 $MNT/f1
> 
> umount $MNT
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> 
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 7421abcf325a..5bb8d8c86311 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -371,6 +371,9 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
>   				   const char *name, const void *buffer,
>   				   size_t size, int flags)
>   {
> +	if (btrfs_root_readonly(BTRFS_I(inode)->root))
> +		return -EROFS;
> +
>   	name = xattr_full_name(handler, name);
>   	return btrfs_setxattr_trans(inode, name, buffer, size, flags);
>   }
>