| Message ID | 20220816214256.t5ikj7pyqe6l6qgn@fiona (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: Check if root is readonly while setting security xattr | expand |
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
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.
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); > } >
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); }