[PATCH] attr: validate kuid first in chown_common

[PATCH] attr: validate kuid first in chown_common

[Su Yue]

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@kernel.org>
Cc: Seth Forshee <sforshee@digitalocean.com>
Fixes: b27c82e12965 ("attr: port attribute changes to new types")
Signed-off-by: Su Yue <glass@fydeos.io>
---
 fs/open.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/open.c b/fs/open.c
index 8a813fa5ca56..967c7aac5aba 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -715,6 +715,13 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
 	mnt_userns = mnt_user_ns(path->mnt);
 	fs_userns = i_user_ns(inode);
 
+	if ((user != (uid_t)-1) &&
+	     !vfsuid_has_fsmapping(mnt_userns, fs_userns, VFSUIDT_INIT(uid)))
+		return -EINVAL;
+	if ((group != (gid_t)-1) &&
+	    !vfsgid_has_fsmapping(mnt_userns, fs_userns, VFSGIDT_INIT(gid)))
+		return -EINVAL;
+
 retry_deleg:
 	newattrs.ia_valid =  ATTR_CTIME;
 	if ((user != (uid_t)-1) && !setattr_vfsuid(&newattrs, uid))
-- 
2.37.1

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

[Christian Brauner]

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@kernel.org>
> Cc: Seth Forshee <sforshee@digitalocean.com>
> Fixes: b27c82e12965 ("attr: port attribute changes to new types")
> Signed-off-by: Su Yue <glass@fydeos.io>
> ---

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?

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

[Su Yue]

On 2022/8/16 18:30, Christian Brauner wrote:
> 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@kernel.org>
>> Cc: Seth Forshee <sforshee@digitalocean.com>
>> Fixes: b27c82e12965 ("attr: port attribute changes to new types")
>> Signed-off-by: Su Yue <glass@fydeos.io>
>> ---
> 
> Thanks for the patch, Su!
> 
Thanks for you quick rely.

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

Agreed. If the errno value is intentional then a fix of fstests case is
the right.

> 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.
> 
As your detailed explanation, EOVERFLOW should be aware of in real word.
Would you like to send a patch to add the above segement to man page of
fchownat(2). EOVERFLOW confused me when I first got the errno.


> Would you care to send something like the following:
> 

Just sent it.

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