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

[Filipe Manana <fdmanana@kernel.org>]

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