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

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

On  9:34 16/08, Filipe Manana wrote:
> On Tue, Aug 16, 2022 at 1:40 AM Goldwyn Rodrigues <rgoldwyn@suse.de> 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.
> 
> Why does that happen only for security xattrs, and not  for xattrs in
> the user.* and btrfs.* namespaces?

xattr_permission() skips checks for security.* and system.*
I will mention it in the next changelog.

> 
> >
> > 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
> 
> A test case only in a changelog isn't super useful to prevent future
> regressions :)
> 
> Can you send a test case for fstests, that also tests user.* and
> btrfs.* namespaces, and creating and deleting xattrs too?

I am merely trying to demonstrate the issue.
I didn't think this warrants a testcase, but if you think so: sure.
However, this case is limited to system.* and security.* tests.

> 
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@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;
> > +
> 
> The same type of check should be done at btrfs_xattr_handler_set_prop() as well.
> Even though trying the same test on a btrfs.compression xattr fails with -EROFS.
> 
> I'm still curious why trying the same on a user.* xattr happens to
> fail with -EROFS,
> but not for a secutiry.* xattr, since both have
> btrfs_xattr_handler_set() as their entry point.
> 
> I think this should be detailed in the changelog, and presume you
> verified why that happens.

See response above.


-- 
Goldwyn