| Message ID | 20220816092538.84252-1-glass@fydeos.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | attr: validate kuid first in chown_common | expand |
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?
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?
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))
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(+)