[PATCH] Check if root is readonly while setting xattr

[PATCH] Check if root is readonly while setting xattr

[Goldwyn Rodrigues]

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 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>


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

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

[Qu Wenruo]

On 2022/8/16 03:34, 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 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>

Thanks,
Qu
>
>
> 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);
>   }
>

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

[Filipe Manana]

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?

>
> 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?

>
> 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.

Thanks.

>         name = xattr_full_name(handler, name);
>         return btrfs_setxattr_trans(inode, name, buffer, size, flags);
>  }
>
> --
> Goldwyn

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

[Goldwyn Rodrigues]

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

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

[Filipe Manana]

On Tue, Aug 16, 2022 at 1:44 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> 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.

Great.
Also, the subject should have a prefix: "btrfs: check if root..."

>
> >
> > >
> > > 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.

Any bug should have a test case (if possible).
I don't expect anyone to keep running this test manually every now and
then to verify if we have a regression :)

> However, this case is limited to system.* and security.* tests.

That doesn't mean we should not have a test case that tests all xattr namespaces
and all xattrs operations (insert, update, delete).

Thanks.

>
> >
> > >
> > > 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