| Message ID | 20220816214051.wsw75y3mtjdsim6w@fiona (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Test xattr changes for read-only btrfs property | expand |
On Tue, Aug 16, 2022 at 04:40:51PM -0500, Goldwyn Rodrigues wrote: > 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 BTRFS read-only property is set to false. > > This tests the bug for "security.*" modifications which escape > xattr_permission(), because security parameters are let through > in xattr_permission(), without checks from > inode_permission()->btrfs_permission(). There is no restriction on > security.* from VFS and decision is left to the underlying filesystem. When a test currently fails and we have a patch that fixes it, we mention it in the changelog and/or the test itself. Nowadays (and I learned this recently as it's new), we have an annotation to add to the test itself, like this: _fixed_by_kernel_commit XXXXXXXXXXXX "btrfs: check if root is readonly while setting security xattr" (Notice that I changed "Check" to "check", because that's the convention for btrfs patches, and David would fix that when picking the patch anyway) In this case since the patch is not yet in Linus' tree, we can leave the XXXXXXXXXXXX marker and later update it once it lands in his tree. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > diff --git a/tests/btrfs/273 b/tests/btrfs/273 > new file mode 100755 > index 00000000..ec7d264d > --- /dev/null > +++ b/tests/btrfs/273 > @@ -0,0 +1,78 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test No. 273 > +# > +# Test that no xattr can be changed once btrfs property is set to RO > +# > +. ./common/preamble > +_begin_fstest auto quick attr subvol group too. > + > +# Import common functions. > +#. ./common/filter Ratther than comment, remove it, but we'll need a filter, see below. > +. ./common/attr > + > +# real QA test starts here > +_supported_fs btrfs > +_require_attrs > +_require_btrfs_command "property" > +_require_scratch > + > +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" > +_scratch_mount > + > +FILENAME=$SCRATCH_MNT/foo > + > +set_xattr() { > + local value=$1 > + $SETFATTR_PROG -n "user.one" -v $value $FILENAME > + $SETFATTR_PROG -n "security.one" -v $value $FILENAME > + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME > +} > + > +get_xattr() { > + $GETFATTR_PROG -n "user.one" $FILENAME > + $GETFATTR_PROG -n "security.one" $FILENAME > + $GETFATTR_PROG -n "trusted.one" $FILENAME > +} > + > +del_xattr() { > + $SETFATTR_PROG -x "user.one" $FILENAME > + $SETFATTR_PROG -x "security.one" $FILENAME > + $SETFATTR_PROG -x "trusted.one" $FILENAME > +} > + > +# 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 is not changed > +get_xattr > + > +# Attempt to remove xattr from RO (property) filesystem > +del_xattr Here we should call get_xattr() again to verify the xattrs were not deleted, just like we did before. > + > +# 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 > + > +status=0 > +exit > diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out > new file mode 100644 > index 00000000..f6fca029 > --- /dev/null > +++ b/tests/btrfs/273.out > @@ -0,0 +1,33 @@ > +QA output created by 273 > +ro=true > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +getfattr: Removing leading '/' from absolute path names We can get rid of this message by passing --absolute-names to getfattr. We do that in every generic test case I can find. > +# file: scratch/foo And --absolute-names is also used in order to be able to filter the golden output. The test fails on any config where SCRATCH_MNT is not "/scratch", like in my environment: -# file: scratch/foo +# file: home/fdmanana/btrfs-tests/scratch_1/foo By passing --absolute-names we can then use _filter_scratch and have this instead in the golden output: # file: SCRATCH_MNT/foo > +user.one="1" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +security.one="1" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +trusted.one="1" > + > +setfattr: /scratch/foo: Read-only file system Same here. The test fails if SCRATCH_MNT is not "/scratch". We need to append "2>&1 | _filter_scratch" to every setfattr call in the test. And then have the following in the golden output instead: setfattr: SCRATCH_MNT/foo: Read-only file system Here's a diff that fixes the golden output and adds the other things mentioned before: diff --git a/tests/btrfs/273 b/tests/btrfs/273 index ec7d264d..37e9e52c 100755 --- a/tests/btrfs/273 +++ b/tests/btrfs/273 @@ -7,14 +7,15 @@ # Test that no xattr can be changed once btrfs property is set to RO # . ./common/preamble -_begin_fstest auto quick attr +_begin_fstest auto quick attr subvol # Import common functions. -#. ./common/filter +. ./common/filter . ./common/attr # real QA test starts here _supported_fs btrfs +_fixed_by_kernel_commit XXXXXXXXXXXX "btrfs: check if root is readonly while setting security xattr" _require_attrs _require_btrfs_command "property" _require_scratch @@ -23,24 +24,23 @@ _scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" _scratch_mount FILENAME=$SCRATCH_MNT/foo - set_xattr() { local value=$1 - $SETFATTR_PROG -n "user.one" -v $value $FILENAME - $SETFATTR_PROG -n "security.one" -v $value $FILENAME - $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME + $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_PROG -n "user.one" $FILENAME - $GETFATTR_PROG -n "security.one" $FILENAME - $GETFATTR_PROG -n "trusted.one" $FILENAME + $GETFATTR_PROG -n "user.one" --absolute-names $FILENAME | _filter_scratch + $GETFATTR_PROG -n "security.one" --absolute-names $FILENAME | _filter_scratch + $GETFATTR_PROG -n "trusted.one" --absolute-names $FILENAME | _filter_scratch } del_xattr() { - $SETFATTR_PROG -x "user.one" $FILENAME - $SETFATTR_PROG -x "security.one" $FILENAME - $SETFATTR_PROG -x "trusted.one" $FILENAME + $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. @@ -54,12 +54,15 @@ $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 is not changed +# Check the values of RO (property) filesystem are not changed get_xattr # Attempt to remove xattr from RO (property) filesystem del_xattr +# Check the values of RO (property) filesystem are not changed +get_xattr + # Change filesystem property RO to false $BTRFS_UTIL_PROG property set $SCRATCH_MNT ro false diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out index f6fca029..ae85b0a5 100644 --- a/tests/btrfs/273.out +++ b/tests/btrfs/273.out @@ -1,33 +1,36 @@ QA output created by 273 ro=true -setfattr: /scratch/foo: Read-only file system -setfattr: /scratch/foo: Read-only file system -setfattr: /scratch/foo: Read-only file system -getfattr: Removing leading '/' from absolute path names -# file: scratch/foo +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" -getfattr: Removing leading '/' from absolute path names -# file: scratch/foo +# file: SCRATCH_MNT/foo security.one="1" -getfattr: Removing leading '/' from absolute path names -# file: scratch/foo +# 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" -setfattr: /scratch/foo: Read-only file system -setfattr: /scratch/foo: Read-only file system -setfattr: /scratch/foo: Read-only file system ro=false -getfattr: Removing leading '/' from absolute path names -# file: scratch/foo +# file: SCRATCH_MNT/foo user.one="2" -getfattr: Removing leading '/' from absolute path names -# file: scratch/foo +# file: SCRATCH_MNT/foo security.one="2" -getfattr: Removing leading '/' from absolute path names -# file: scratch/foo +# file: SCRATCH_MNT/foo trusted.one="2" Thanks. > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +ro=false > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +user.one="2" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +security.one="2" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +trusted.one="2" > + > > -- > Goldwyn
On 11:51 17/08, Filipe Manana wrote: > On Tue, Aug 16, 2022 at 04:40:51PM -0500, Goldwyn Rodrigues wrote: > > 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 BTRFS read-only property is set to false. > > > > This tests the bug for "security.*" modifications which escape > > xattr_permission(), because security parameters are let through > > in xattr_permission(), without checks from > > inode_permission()->btrfs_permission(). There is no restriction on > > security.* from VFS and decision is left to the underlying filesystem. > > When a test currently fails and we have a patch that fixes it, we mention > it in the changelog and/or the test itself. > > Nowadays (and I learned this recently as it's new), we have an annotation > to add to the test itself, like this: > > _fixed_by_kernel_commit XXXXXXXXXXXX "btrfs: check if root is readonly while setting security xattr" > > (Notice that I changed "Check" to "check", because that's the convention > for btrfs patches, and David would fix that when picking the patch anyway) > > In this case since the patch is not yet in Linus' tree, we can leave > the XXXXXXXXXXXX marker and later update it once it lands in his tree. > > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > diff --git a/tests/btrfs/273 b/tests/btrfs/273 > > new file mode 100755 > > index 00000000..ec7d264d > > --- /dev/null > > +++ b/tests/btrfs/273 > > @@ -0,0 +1,78 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > > +# > > +# FS QA Test No. 273 > > +# > > +# Test that no xattr can be changed once btrfs property is set to RO > > +# > > +. ./common/preamble > > +_begin_fstest auto quick attr > > subvol group too. > > > + > > +# Import common functions. > > +#. ./common/filter > > Ratther than comment, remove it, but we'll need a filter, see below. > > > +. ./common/attr > > + > > +# real QA test starts here > > +_supported_fs btrfs > > +_require_attrs > > +_require_btrfs_command "property" > > +_require_scratch > > + > > +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" > > +_scratch_mount > > + > > +FILENAME=$SCRATCH_MNT/foo > > + > > +set_xattr() { > > + local value=$1 > > + $SETFATTR_PROG -n "user.one" -v $value $FILENAME > > + $SETFATTR_PROG -n "security.one" -v $value $FILENAME > > + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME > > +} > > + > > +get_xattr() { > > + $GETFATTR_PROG -n "user.one" $FILENAME > > + $GETFATTR_PROG -n "security.one" $FILENAME > > + $GETFATTR_PROG -n "trusted.one" $FILENAME > > +} > > + > > +del_xattr() { > > + $SETFATTR_PROG -x "user.one" $FILENAME > > + $SETFATTR_PROG -x "security.one" $FILENAME > > + $SETFATTR_PROG -x "trusted.one" $FILENAME > > +} > > + > > +# 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 is not changed > > +get_xattr > > + > > +# Attempt to remove xattr from RO (property) filesystem > > +del_xattr > > Here we should call get_xattr() again to verify the xattrs were > not deleted, just like we did before. > This should be called after they are truly deleted once the filesystem is RW. I will add that. > > + > > +# 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 > > + > > +status=0 > > +exit > > diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out > > new file mode 100644 > > index 00000000..f6fca029 > > --- /dev/null > > +++ b/tests/btrfs/273.out > > @@ -0,0 +1,33 @@ > > +QA output created by 273 > > +ro=true > > +setfattr: /scratch/foo: Read-only file system > > +setfattr: /scratch/foo: Read-only file system > > +setfattr: /scratch/foo: Read-only file system > > +getfattr: Removing leading '/' from absolute path names > > We can get rid of this message by passing --absolute-names to getfattr. > We do that in every generic test case I can find. > > > +# file: scratch/foo > > And --absolute-names is also used in order to be able to filter the > golden output. > > The test fails on any config where SCRATCH_MNT is not "/scratch", > like in my environment: > > -# file: scratch/foo > +# file: home/fdmanana/btrfs-tests/scratch_1/foo > > By passing --absolute-names we can then use _filter_scratch and have > this instead in the golden output: > > # file: SCRATCH_MNT/foo Yes, missed that.
On 8/17/22 05:40, Goldwyn Rodrigues wrote: > 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 BTRFS read-only property is set to false. > > This tests the bug for "security.*" modifications which escape > xattr_permission(), because security parameters are let through > in xattr_permission(), without checks from > inode_permission()->btrfs_permission(). There is no restriction on > security.* from VFS and decision is left to the underlying filesystem. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > diff --git a/tests/btrfs/273 b/tests/btrfs/273 > new file mode 100755 > index 00000000..ec7d264d > --- /dev/null > +++ b/tests/btrfs/273 > @@ -0,0 +1,78 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test No. 273 > +# > +# 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 > +_require_attrs > +_require_btrfs_command "property" > +_require_scratch > + > +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" > +_scratch_mount > + > +FILENAME=$SCRATCH_MNT/foo > + > +set_xattr() { > + local value=$1 > + $SETFATTR_PROG -n "user.one" -v $value $FILENAME > + $SETFATTR_PROG -n "security.one" -v $value $FILENAME > + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME > +} > + > +get_xattr() { > + $GETFATTR_PROG -n "user.one" $FILENAME > + $GETFATTR_PROG -n "security.one" $FILENAME > + $GETFATTR_PROG -n "trusted.one" $FILENAME > +} > + > +del_xattr() { > + $SETFATTR_PROG -x "user.one" $FILENAME > + $SETFATTR_PROG -x "security.one" $FILENAME > + $SETFATTR_PROG -x "trusted.one" $FILENAME > +} > + This output contains mnt references that need to be filtered. Filter _filter_scratch should help. > +# 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 is not changed > +get_xattr > + > +# Attempt to remove xattr from RO (property) filesystem > +del_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 > + Nit: We are reusing the value "2" and changing it to "3" makes it unique and so the debugging easier. > +# Get the changed values > +get_xattr > + > +# Remove xattr > +del_xattr > + > +status=0 > +exit > diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out > new file mode 100644 > index 00000000..f6fca029 > --- /dev/null > +++ b/tests/btrfs/273.out > @@ -0,0 +1,33 @@ > +QA output created by 273 > +ro=true > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +user.one="1" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +security.one="1" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +trusted.one="1" > + > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +setfattr: /scratch/foo: Read-only file system > +ro=false > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +user.one="2" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +security.one="2" > + > +getfattr: Removing leading '/' from absolute path names > +# file: scratch/foo > +trusted.one="2" > + Nit: A whitespace. Thanks.
On Thu, Aug 18, 2022 at 4:08 AM Anand Jain <anand.jain@oracle.com> wrote: > > > > > On 8/17/22 05:40, Goldwyn Rodrigues wrote: > > 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 BTRFS read-only property is set to false. > > > > This tests the bug for "security.*" modifications which escape > > xattr_permission(), because security parameters are let through > > in xattr_permission(), without checks from > > inode_permission()->btrfs_permission(). There is no restriction on > > security.* from VFS and decision is left to the underlying filesystem. > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > diff --git a/tests/btrfs/273 b/tests/btrfs/273 > > new file mode 100755 > > index 00000000..ec7d264d > > --- /dev/null > > +++ b/tests/btrfs/273 > > @@ -0,0 +1,78 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > > +# > > +# FS QA Test No. 273 > > +# > > +# 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 > > +_require_attrs > > +_require_btrfs_command "property" > > +_require_scratch > > + > > +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" > > +_scratch_mount > > + > > +FILENAME=$SCRATCH_MNT/foo > > + > > +set_xattr() { > > + local value=$1 > > + $SETFATTR_PROG -n "user.one" -v $value $FILENAME > > + $SETFATTR_PROG -n "security.one" -v $value $FILENAME > > + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME > > +} > > + > > +get_xattr() { > > + $GETFATTR_PROG -n "user.one" $FILENAME > > + $GETFATTR_PROG -n "security.one" $FILENAME > > + $GETFATTR_PROG -n "trusted.one" $FILENAME > > +} > > + > > +del_xattr() { > > + $SETFATTR_PROG -x "user.one" $FILENAME > > + $SETFATTR_PROG -x "security.one" $FILENAME > > + $SETFATTR_PROG -x "trusted.one" $FILENAME > > +} > > + > > This output contains mnt references that need to be filtered. > Filter _filter_scratch should help. That was already pointed out in my previous review, wasn't it? > > > > > +# 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 is not changed > > +get_xattr > > + > > +# Attempt to remove xattr from RO (property) filesystem > > +del_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 > > + > > Nit: We are reusing the value "2" and changing it to "3" makes it > unique and so the debugging easier. That's a good idea. > > > > +# Get the changed values > > +get_xattr > > + > > +# Remove xattr > > +del_xattr > > + > > +status=0 > > +exit > > diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out > > new file mode 100644 > > index 00000000..f6fca029 > > --- /dev/null > > +++ b/tests/btrfs/273.out > > @@ -0,0 +1,33 @@ > > +QA output created by 273 > > +ro=true > > +setfattr: /scratch/foo: Read-only file system > > +setfattr: /scratch/foo: Read-only file system > > +setfattr: /scratch/foo: Read-only file system > > +getfattr: Removing leading '/' from absolute path names > > +# file: scratch/foo > > +user.one="1" > > + > > +getfattr: Removing leading '/' from absolute path names > > +# file: scratch/foo > > +security.one="1" > > + > > +getfattr: Removing leading '/' from absolute path names > > +# file: scratch/foo > > +trusted.one="1" > > + > > +setfattr: /scratch/foo: Read-only file system > > +setfattr: /scratch/foo: Read-only file system > > +setfattr: /scratch/foo: Read-only file system > > +ro=false > > +getfattr: Removing leading '/' from absolute path names > > +# file: scratch/foo > > +user.one="2" > > + > > +getfattr: Removing leading '/' from absolute path names > > +# file: scratch/foo > > +security.one="2" > > + > > +getfattr: Removing leading '/' from absolute path names > > +# file: scratch/foo > > +trusted.one="2" > > > > + > > Nit: A whitespace. It's needed, getfattr prints a blank line at the end. > > Thanks. > >
On 8/18/22 16:09, Filipe Manana wrote: > On Thu, Aug 18, 2022 at 4:08 AM Anand Jain <anand.jain@oracle.com> wrote: >> >> >> >> >> On 8/17/22 05:40, Goldwyn Rodrigues wrote: >>> 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 BTRFS read-only property is set to false. >>> >>> This tests the bug for "security.*" modifications which escape >>> xattr_permission(), because security parameters are let through >>> in xattr_permission(), without checks from >>> inode_permission()->btrfs_permission(). There is no restriction on >>> security.* from VFS and decision is left to the underlying filesystem. >>> >>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> >>> >>> diff --git a/tests/btrfs/273 b/tests/btrfs/273 >>> new file mode 100755 >>> index 00000000..ec7d264d >>> --- /dev/null >>> +++ b/tests/btrfs/273 >>> @@ -0,0 +1,78 @@ >>> +#! /bin/bash >>> +# SPDX-License-Identifier: GPL-2.0 >>> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. >>> +# >>> +# FS QA Test No. 273 >>> +# >>> +# 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 >>> +_require_attrs >>> +_require_btrfs_command "property" >>> +_require_scratch >>> + >>> +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" >>> +_scratch_mount >>> + >>> +FILENAME=$SCRATCH_MNT/foo >>> + >>> +set_xattr() { >>> + local value=$1 >>> + $SETFATTR_PROG -n "user.one" -v $value $FILENAME >>> + $SETFATTR_PROG -n "security.one" -v $value $FILENAME >>> + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME >>> +} >>> + >>> +get_xattr() { >>> + $GETFATTR_PROG -n "user.one" $FILENAME >>> + $GETFATTR_PROG -n "security.one" $FILENAME >>> + $GETFATTR_PROG -n "trusted.one" $FILENAME >>> +} >>> + >>> +del_xattr() { >>> + $SETFATTR_PROG -x "user.one" $FILENAME >>> + $SETFATTR_PROG -x "security.one" $FILENAME >>> + $SETFATTR_PROG -x "trusted.one" $FILENAME >>> +} >>> + >> >> This output contains mnt references that need to be filtered. >> Filter _filter_scratch should help. > > That was already pointed out in my previous review, wasn't it? Oh right. I missed it. > >> >> >> >>> +# 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 is not changed >>> +get_xattr >>> + >>> +# Attempt to remove xattr from RO (property) filesystem >>> +del_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 >>> + >> >> Nit: We are reusing the value "2" and changing it to "3" makes it >> unique and so the debugging easier. > > That's a good idea. > >> >> >>> +# Get the changed values >>> +get_xattr >>> + >>> +# Remove xattr >>> +del_xattr >>> + >>> +status=0 >>> +exit >>> diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out >>> new file mode 100644 >>> index 00000000..f6fca029 >>> --- /dev/null >>> +++ b/tests/btrfs/273.out >>> @@ -0,0 +1,33 @@ >>> +QA output created by 273 >>> +ro=true >>> +setfattr: /scratch/foo: Read-only file system >>> +setfattr: /scratch/foo: Read-only file system >>> +setfattr: /scratch/foo: Read-only file system >>> +getfattr: Removing leading '/' from absolute path names >>> +# file: scratch/foo >>> +user.one="1" >>> + >>> +getfattr: Removing leading '/' from absolute path names >>> +# file: scratch/foo >>> +security.one="1" >>> + >>> +getfattr: Removing leading '/' from absolute path names >>> +# file: scratch/foo >>> +trusted.one="1" >>> + >>> +setfattr: /scratch/foo: Read-only file system >>> +setfattr: /scratch/foo: Read-only file system >>> +setfattr: /scratch/foo: Read-only file system >>> +ro=false >>> +getfattr: Removing leading '/' from absolute path names >>> +# file: scratch/foo >>> +user.one="2" >>> + >>> +getfattr: Removing leading '/' from absolute path names >>> +# file: scratch/foo >>> +security.one="2" >>> + >>> +getfattr: Removing leading '/' from absolute path names >>> +# file: scratch/foo >>> +trusted.one="2" >> >> >>> + >> >> Nit: A whitespace. > > It's needed, getfattr prints a blank line at the end. > Ok. Thanks. >> >> Thanks. >> >>
diff --git a/tests/btrfs/273 b/tests/btrfs/273 new file mode 100755 index 00000000..ec7d264d --- /dev/null +++ b/tests/btrfs/273 @@ -0,0 +1,78 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. +# +# FS QA Test No. 273 +# +# 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 +_require_attrs +_require_btrfs_command "property" +_require_scratch + +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed" +_scratch_mount + +FILENAME=$SCRATCH_MNT/foo + +set_xattr() { + local value=$1 + $SETFATTR_PROG -n "user.one" -v $value $FILENAME + $SETFATTR_PROG -n "security.one" -v $value $FILENAME + $SETFATTR_PROG -n "trusted.one" -v $value $FILENAME +} + +get_xattr() { + $GETFATTR_PROG -n "user.one" $FILENAME + $GETFATTR_PROG -n "security.one" $FILENAME + $GETFATTR_PROG -n "trusted.one" $FILENAME +} + +del_xattr() { + $SETFATTR_PROG -x "user.one" $FILENAME + $SETFATTR_PROG -x "security.one" $FILENAME + $SETFATTR_PROG -x "trusted.one" $FILENAME +} + +# 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 is not changed +get_xattr + +# Attempt to remove xattr from RO (property) filesystem +del_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 + +status=0 +exit diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out new file mode 100644 index 00000000..f6fca029 --- /dev/null +++ b/tests/btrfs/273.out @@ -0,0 +1,33 @@ +QA output created by 273 +ro=true +setfattr: /scratch/foo: Read-only file system +setfattr: /scratch/foo: Read-only file system +setfattr: /scratch/foo: Read-only file system +getfattr: Removing leading '/' from absolute path names +# file: scratch/foo +user.one="1" + +getfattr: Removing leading '/' from absolute path names +# file: scratch/foo +security.one="1" + +getfattr: Removing leading '/' from absolute path names +# file: scratch/foo +trusted.one="1" + +setfattr: /scratch/foo: Read-only file system +setfattr: /scratch/foo: Read-only file system +setfattr: /scratch/foo: Read-only file system +ro=false +getfattr: Removing leading '/' from absolute path names +# file: scratch/foo +user.one="2" + +getfattr: Removing leading '/' from absolute path names +# file: scratch/foo +security.one="2" + +getfattr: Removing leading '/' from absolute path names +# file: scratch/foo +trusted.one="2" +
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 BTRFS read-only property is set to false. This tests the bug for "security.*" modifications which escape xattr_permission(), because security parameters are let through in xattr_permission(), without checks from inode_permission()->btrfs_permission(). There is no restriction on security.* from VFS and decision is left to the underlying filesystem. Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>