* [PATCH v3] btrfs: test xattr changes for RO btrfs property
@ 2022-09-22 9:56 fdmanana
2022-09-23 14:46 ` Zorro Lang
0 siblings, 1 reply; 2+ messages in thread
From: fdmanana @ 2022-09-22 9:56 UTC (permalink / raw)
To: fstests; +Cc: linux-btrfs, Goldwyn Rodrigues, Filipe Manana, Goldwyn Rodrigues
From: Goldwyn Rodrigues <rgoldwyn@suse.de>
Test creation, modification and deletion of xattr for a BTRFS filesystem
which has the read-only property set to true.
Re-test the same after read-only property is set to false.
This tests the bug for "security.*" modifications which escape
xattr_permission(), because security parameters are usually let through
in xattr_permission, without checking
inode_permission()->btrfs_permission().
This resulted in being able to change a security xattr on a RO subvolume
until it got fixed by kernel commit b51111271b03 ("btrfs: check if root
is readonly while setting security xattr").
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
V3: Use the _getfattr() helper.
Redirect mkfs output to the .full file and remove the _fail call,
which is an antipattern.
Change the subject from:
"btrfs: test security xattr changes for RO btrfs property"
to:
"btrfs: test xattr changes for RO btrfs property"
Since it does not test only security xattrs.
tests/btrfs/275 | 88 +++++++++++++++++++++++++++++++++++++++++++++
tests/btrfs/275.out | 39 ++++++++++++++++++++
2 files changed, 127 insertions(+)
create mode 100755 tests/btrfs/275
create mode 100644 tests/btrfs/275.out
diff --git a/tests/btrfs/275 b/tests/btrfs/275
new file mode 100755
index 00000000..b34fbda0
--- /dev/null
+++ b/tests/btrfs/275
@@ -0,0 +1,88 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test No. 275
+#
+# Test that no xattr can be changed once btrfs property is set to RO.
+#
+. ./common/preamble
+_begin_fstest auto quick attr
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+_supported_fs btrfs
+_fixed_by_kernel_commit b51111271b03 \
+ "btrfs: check if root is readonly while setting security xattr"
+_require_attrs
+_require_btrfs_command "property"
+_require_scratch
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+FILENAME=$SCRATCH_MNT/foo
+
+set_xattr()
+{
+ local value=$1
+ $SETFATTR_PROG -n "user.one" -v $value $FILENAME 2>&1 | _filter_scratch
+ $SETFATTR_PROG -n "security.one" -v $value $FILENAME 2>&1 | _filter_scratch
+ $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME 2>&1 | _filter_scratch
+}
+
+get_xattr()
+{
+ _getfattr --absolute-names -n "user.one" $FILENAME 2>&1 | _filter_scratch
+ _getfattr --absolute-names -n "security.one" $FILENAME 2>&1 | _filter_scratch
+ _getfattr --absolute-names -n "trusted.one" $FILENAME 2>&1 | _filter_scratch
+}
+
+del_xattr()
+{
+ $SETFATTR_PROG -x "user.one" $FILENAME 2>&1 | _filter_scratch
+ $SETFATTR_PROG -x "security.one" $FILENAME 2>&1 | _filter_scratch
+ $SETFATTR_PROG -x "trusted.one" $FILENAME 2>&1 | _filter_scratch
+}
+
+# Create a test file.
+echo "hello world" > $FILENAME
+
+set_xattr 1
+
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro true
+$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
+
+# Attempt to change values of RO (property) filesystem.
+set_xattr 2
+
+# Check the values of RO (property) filesystem are not changed.
+get_xattr
+
+# Attempt to remove xattr from RO (property) filesystem.
+del_xattr
+
+# Check if xattr still exist.
+get_xattr
+
+# Change filesystem property RO to false
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro false
+$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
+
+# Change the xattrs after RO is false.
+set_xattr 2
+
+# Get the changed values.
+get_xattr
+
+# Remove xattr.
+del_xattr
+
+# check if the xattrs are really deleted.
+get_xattr
+
+status=0
+exit
diff --git a/tests/btrfs/275.out b/tests/btrfs/275.out
new file mode 100644
index 00000000..fb8f02f8
--- /dev/null
+++ b/tests/btrfs/275.out
@@ -0,0 +1,39 @@
+QA output created by 275
+ro=true
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+# file: SCRATCH_MNT/foo
+user.one="1"
+
+# file: SCRATCH_MNT/foo
+security.one="1"
+
+# file: SCRATCH_MNT/foo
+trusted.one="1"
+
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+# file: SCRATCH_MNT/foo
+user.one="1"
+
+# file: SCRATCH_MNT/foo
+security.one="1"
+
+# file: SCRATCH_MNT/foo
+trusted.one="1"
+
+ro=false
+# file: SCRATCH_MNT/foo
+user.one="2"
+
+# file: SCRATCH_MNT/foo
+security.one="2"
+
+# file: SCRATCH_MNT/foo
+trusted.one="2"
+
+SCRATCH_MNT/foo: user.one: No such attribute
+SCRATCH_MNT/foo: security.one: No such attribute
+SCRATCH_MNT/foo: trusted.one: No such attribute
--
2.35.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] btrfs: test xattr changes for RO btrfs property
2022-09-22 9:56 [PATCH v3] btrfs: test xattr changes for RO btrfs property fdmanana
@ 2022-09-23 14:46 ` Zorro Lang
0 siblings, 0 replies; 2+ messages in thread
From: Zorro Lang @ 2022-09-23 14:46 UTC (permalink / raw)
To: fdmanana
Cc: fstests, linux-btrfs, Goldwyn Rodrigues, Filipe Manana,
Goldwyn Rodrigues
On Thu, Sep 22, 2022 at 10:56:39AM +0100, fdmanana@kernel.org wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.de>
>
> Test creation, modification and deletion of xattr for a BTRFS filesystem
> which has the read-only property set to true.
>
> Re-test the same after read-only property is set to false.
>
> This tests the bug for "security.*" modifications which escape
> xattr_permission(), because security parameters are usually let through
> in xattr_permission, without checking
> inode_permission()->btrfs_permission().
>
> This resulted in being able to change a security xattr on a RO subvolume
> until it got fixed by kernel commit b51111271b03 ("btrfs: check if root
> is readonly while setting security xattr").
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>
> V3: Use the _getfattr() helper.
> Redirect mkfs output to the .full file and remove the _fail call,
> which is an antipattern.
>
> Change the subject from:
>
> "btrfs: test security xattr changes for RO btrfs property"
>
> to:
>
> "btrfs: test xattr changes for RO btrfs property"
>
> Since it does not test only security xattrs.
This version looks good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>
>
> tests/btrfs/275 | 88 +++++++++++++++++++++++++++++++++++++++++++++
> tests/btrfs/275.out | 39 ++++++++++++++++++++
> 2 files changed, 127 insertions(+)
> create mode 100755 tests/btrfs/275
> create mode 100644 tests/btrfs/275.out
>
> diff --git a/tests/btrfs/275 b/tests/btrfs/275
> new file mode 100755
> index 00000000..b34fbda0
> --- /dev/null
> +++ b/tests/btrfs/275
> @@ -0,0 +1,88 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test No. 275
> +#
> +# Test that no xattr can be changed once btrfs property is set to RO.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick attr
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +
> +# real QA test starts here
> +_supported_fs btrfs
> +_fixed_by_kernel_commit b51111271b03 \
> + "btrfs: check if root is readonly while setting security xattr"
> +_require_attrs
> +_require_btrfs_command "property"
> +_require_scratch
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount
> +
> +FILENAME=$SCRATCH_MNT/foo
> +
> +set_xattr()
> +{
> + local value=$1
> + $SETFATTR_PROG -n "user.one" -v $value $FILENAME 2>&1 | _filter_scratch
> + $SETFATTR_PROG -n "security.one" -v $value $FILENAME 2>&1 | _filter_scratch
> + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME 2>&1 | _filter_scratch
> +}
> +
> +get_xattr()
> +{
> + _getfattr --absolute-names -n "user.one" $FILENAME 2>&1 | _filter_scratch
> + _getfattr --absolute-names -n "security.one" $FILENAME 2>&1 | _filter_scratch
> + _getfattr --absolute-names -n "trusted.one" $FILENAME 2>&1 | _filter_scratch
> +}
> +
> +del_xattr()
> +{
> + $SETFATTR_PROG -x "user.one" $FILENAME 2>&1 | _filter_scratch
> + $SETFATTR_PROG -x "security.one" $FILENAME 2>&1 | _filter_scratch
> + $SETFATTR_PROG -x "trusted.one" $FILENAME 2>&1 | _filter_scratch
> +}
> +
> +# Create a test file.
> +echo "hello world" > $FILENAME
> +
> +set_xattr 1
> +
> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro true
> +$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
> +
> +# Attempt to change values of RO (property) filesystem.
> +set_xattr 2
> +
> +# Check the values of RO (property) filesystem are not changed.
> +get_xattr
> +
> +# Attempt to remove xattr from RO (property) filesystem.
> +del_xattr
> +
> +# Check if xattr still exist.
> +get_xattr
> +
> +# Change filesystem property RO to false
> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro false
> +$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
> +
> +# Change the xattrs after RO is false.
> +set_xattr 2
> +
> +# Get the changed values.
> +get_xattr
> +
> +# Remove xattr.
> +del_xattr
> +
> +# check if the xattrs are really deleted.
> +get_xattr
> +
> +status=0
> +exit
> diff --git a/tests/btrfs/275.out b/tests/btrfs/275.out
> new file mode 100644
> index 00000000..fb8f02f8
> --- /dev/null
> +++ b/tests/btrfs/275.out
> @@ -0,0 +1,39 @@
> +QA output created by 275
> +ro=true
> +setfattr: SCRATCH_MNT/foo: Read-only file system
> +setfattr: SCRATCH_MNT/foo: Read-only file system
> +setfattr: SCRATCH_MNT/foo: Read-only file system
> +# file: SCRATCH_MNT/foo
> +user.one="1"
> +
> +# file: SCRATCH_MNT/foo
> +security.one="1"
> +
> +# file: SCRATCH_MNT/foo
> +trusted.one="1"
> +
> +setfattr: SCRATCH_MNT/foo: Read-only file system
> +setfattr: SCRATCH_MNT/foo: Read-only file system
> +setfattr: SCRATCH_MNT/foo: Read-only file system
> +# file: SCRATCH_MNT/foo
> +user.one="1"
> +
> +# file: SCRATCH_MNT/foo
> +security.one="1"
> +
> +# file: SCRATCH_MNT/foo
> +trusted.one="1"
> +
> +ro=false
> +# file: SCRATCH_MNT/foo
> +user.one="2"
> +
> +# file: SCRATCH_MNT/foo
> +security.one="2"
> +
> +# file: SCRATCH_MNT/foo
> +trusted.one="2"
> +
> +SCRATCH_MNT/foo: user.one: No such attribute
> +SCRATCH_MNT/foo: security.one: No such attribute
> +SCRATCH_MNT/foo: trusted.one: No such attribute
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-09-23 14:46 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 9:56 [PATCH v3] btrfs: test xattr changes for RO btrfs property fdmanana
2022-09-23 14:46 ` Zorro Lang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).