Re: [PATCH] attr: validate kuid first in chown_common

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 16, 2022 at 05:25:38PM +0800, Su Yue wrote:
> Since the commit b27c82e12965 ("attr: port attribute changes to new
> types"), chown_common stores vfs{g,u}id which converted from kuid into
> iattr::vfs{g,u}id without check of the corresponding fs mapping ids.
> 
> When fchownat(2) is called with unmapped {g,u}id, now chown_common
> fails later by vfsuid_has_fsmapping in notify_change. Then it returns
> EOVERFLOW instead of EINVAL to the caller.
> 
> Fix it by validating k{u,g}id whether has valid fs mapping ids in
> chown_common so it can return EINVAL early and make fchownat(2)
> behave consistently.
> 
> This commit fixes fstests/generic/656.
> 
> Cc: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> Cc: Seth Forshee <sforshee@xxxxxxxxxxxxxxxx>
> Fixes: b27c82e12965 ("attr: port attribute changes to new types")
> Signed-off-by: Su Yue <glass@xxxxxxxxx>
> ---

Thanks for the patch, Su!

I'm aware of this change in behavior and it is intentional. The
regression risk outside of fstests is very low. So I would prefer if we
fix the test in fstests first to check for EINVAL or EOVERFLOW.

The reason is that reporting EOVERFLOW for this case is the correct
behavior imho:

- EINVAL should only be reported because the target {g,u}id_t has no
  mapping in the caller's idmapping, i.e. doesn't yield a valid k{g,u}id_t.
- EOVERFLOW should be reported because the target k{g,u}id_t doesn't
  have a mapping in the filesystem idmapping or mount idmapping. IOW,
  the filesystem cannot represent the intended value. The mount's
  idmapping is on a par with the filesystem idmapping and thus a failure
  to represent a vfs{g,u}id_t in the filesystem should yield EOVERFLOW.

Would you care to send something like the following:

diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
index 63297d5f..ee41110f 100644
--- a/src/vfs/idmapped-mounts.c
+++ b/src/vfs/idmapped-mounts.c
@@ -7367,7 +7367,7 @@ static int setattr_fix_968219708108(const struct vfstest_info *info)
                 */
                if (!fchownat(open_tree_fd, FILE1, 0, 0, AT_SYMLINK_NOFOLLOW))
                        die("failure: change ownership");
-               if (errno != EINVAL)
+               if (errno != EINVAL && errno != EOVERFLOW)
                        die("failure: errno");

                /*
@@ -7457,7 +7457,7 @@ static int setattr_fix_968219708108(const struct vfstest_info *info)
                 */
                if (!fchownat(open_tree_fd, FILE1, 0, 0, AT_SYMLINK_NOFOLLOW))
                        die("failure: change ownership");
-               if (errno != EINVAL)
+               if (errno != EINVAL && errno != EOVERFLOW)
                        die("failure: errno");

                /*

to fstests upstream?



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux