Re: [RFC PATCH v2 21/41] fs: add chown_r(), lchown_r(), fchown_r() and fchownat_r()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] fs: broken unchanged and malformed ID handling in chown_path() for !MULTIUSER
- [High] fs: missing s_user_ns mapping check for UUID/GUID in chown_path()
- [Medium] fs: chown_ids_r() accepts nonzero reserved bytes for UID_R_NONE and -1
--

commit c1c7ee0a217eb922615794b81d3b58341468ee6f
Author: Jan Koester <jan.koester@tuxist.de>

fs: add chown_r(), lchown_r(), fchown_r() and fchownat_r()

This commit adds the chown_r(), lchown_r(), fchown_r(), and fchownat_r()
system calls accepting struct __kernel_uid_r owners and groups, and refactors
the classic chown paths to share lookup and setattr logic via struct
chown_ids and chown_path().

> diff --git a/fs/open.c b/fs/open.c
> index 6b1c14e684a93..26175b5aa8aa6 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -785,7 +785,46 @@ static inline bool setattr_vfsgid(struct iattr *attr, kgid_t kgid)
>  	return true;
>  }
>  
> -int chown_common(const struct path *path, uid_t user, gid_t group)
> +/*
> + * The new owner and group of a chown. Invalid ids stand for "unchanged";
> + * @err is an error of their conversion, reported once the path is found,
> + * which keeps the classic calls' order of errors.
> + */
> +struct chown_ids {
> +	kuid_t uid;
> +	kgid_t gid;
> +	int err;
> +};
> +
> +static struct chown_ids chown_ids_classic(uid_t user, gid_t group)
> +{
> +	struct user_namespace *ns = current_user_ns();
> +	struct chown_ids ids = {
> +		.uid = make_kuid(ns, user),
> +		.gid = make_kgid(ns, group),
> +	};
> +
> +	if ((user != (uid_t)-1 && !uid_valid(ids.uid)) ||
> +	    (group != (gid_t)-1 && !gid_valid(ids.gid)))
> +		ids.err = -EINVAL;
> +	return ids;
> +}
> +
> +static int chown_ids_r(const struct __kernel_uid_r __user *user,
> +		       const struct __kernel_uid_r __user *group,
> +		       struct chown_ids *ids)
> +{
> +	struct user_namespace *ns = current_user_ns();
> +	int err;
> +
> +	ids->err = 0;
> +	err = get_kuid_r(ns, user, &ids->uid);
> +	if (!err)
> +		err = get_kgid_r(ns, group, &ids->gid);
> +	return err;
> +}

[Severity: Medium]
When userspace calls sys_chown_r(), sys_lchown_r(), sys_fchown_r(), or
sys_fchownat_r() with a struct __kernel_uid_r whose type is UID_R_NONE or
UID_R_LEGACY with legacy == (u32)-1, can non-zero bytes in the reserved union
area bypass validation?

In include/uapi/linux/uidgid_r.h, struct __kernel_uid_r documents:

    /*
     * Bytes past the member .type selects are reserved and must be zero.
     */

However, chown_ids_r() delegates to get_kuid_r() and get_kgid_r() in
kernel/uidgid_r.c, which return 0 as soon as kid_r_is_none(&id) matches,
before make_kuid_r() or make_kgid_r() calls kid_r_valid(&id):

    static bool kid_r_is_none(const struct __kernel_uid_r *id)
    {
            return id->type == UID_R_NONE ||
                   (id->type == UID_R_LEGACY && id->legacy == (u32)-1);
    }

    int get_kuid_r(struct user_namespace *from,
                   const struct __kernel_uid_r __user *uid, kuid_t *kuid)
    {
            ...
            if (kid_r_is_none(&id))
                    return 0;
            *kuid = make_kuid_r(from, &id);
            return uid_valid(*kuid) ? 0 : -EINVAL;
    }

Because kid_r_is_none() does not check the 16 union bytes for UID_R_NONE or
the 12 trailing bytes (id->uuid + sizeof(id->legacy)) for UID_R_LEGACY with
legacy == (u32)-1, shouldn't those reserved bytes be verified to be zero so
that non-zero trailing bytes are rejected with -EINVAL?

> +
> +static int chown_path(const struct path *path, const struct chown_ids *ids)
>  {
>  	struct mnt_idmap *idmap;
>  	struct user_namespace *fs_userns;
> @@ -793,11 +832,9 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
>  	struct delegated_inode delegated_inode = { };
>  	int error;
>  	struct iattr newattrs;
> -	kuid_t uid;
> -	kgid_t gid;
>  
> -	uid = make_kuid(current_user_ns(), user);
> -	gid = make_kgid(current_user_ns(), group);
> +	if (ids->err)
> +		return ids->err;
>  
>  	idmap = mnt_idmap(path->mnt);
>  	fs_userns = i_user_ns(inode);
> @@ -806,9 +843,9 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
>  	newattrs.ia_vfsuid = INVALID_VFSUID;
>  	newattrs.ia_vfsgid = INVALID_VFSGID;
>  	newattrs.ia_valid =  ATTR_CTIME;
> -	if ((user != (uid_t)-1) && !setattr_vfsuid(&newattrs, uid))
> +	if (uid_valid(ids->uid) && !setattr_vfsuid(&newattrs, ids->uid))
>  		return -EINVAL;
> -	if ((group != (gid_t)-1) && !setattr_vfsgid(&newattrs, gid))
> +	if (gid_valid(ids->gid) && !setattr_vfsgid(&newattrs, ids->gid))
>  		return -EINVAL;

[Severity: High]
How does chown_path() handle unchanged IDs (-1, NULL, or UID_R_NONE) and
malformed IDs when CONFIG_MULTIUSER is disabled?

Previously, chown_common() checked (user != (uid_t)-1) and
(group != (gid_t)-1) before calling setattr_vfsuid() and setattr_vfsgid().
With this change, chown_ids_classic() and chown_ids_r() encode an unchanged
owner or group as INVALID_UID / INVALID_GID, and chown_path() uses
uid_valid(ids->uid) and gid_valid(ids->gid) to decide whether to set ATTR_UID
and ATTR_GID.

In include/linux/uidgid.h, when !CONFIG_MULTIUSER, __kuid_val() and
__kgid_val() always return 0:

    static inline uid_t __kuid_val(kuid_t uid)
    {
            return 0;
    }

    static inline bool uid_valid(kuid_t uid)
    {
            return !uid_is_legacy(uid) || __kuid_val(uid) != (uid_t) -1;
    }

As a result, uid_valid(INVALID_UID) and gid_valid(INVALID_GID) always return
true on !CONFIG_MULTIUSER builds:

1. Calling chown(), lchown(), fchown(), or fchownat() with -1, or calling
   chown_r(), lchown_r(), fchown_r(), or fchownat_r() with NULL, UID_R_NONE,
   or legacy -1 causes chown_path() to call setattr_vfsuid() and
   setattr_vfsgid() with INVALID_UID and INVALID_GID. That sets ATTR_UID and
   ATTR_GID in newattrs.ia_valid and copies VFSUIDT_INIT(INVALID_UID) and
   VFSGIDT_INIT(INVALID_GID) (with .legacy = -1) into newattrs.ia_vfsuid and
   newattrs.ia_vfsgid, causing notify_change() -> may_setattr() to reject
   no-op chown calls on immutable/append-only files with -EPERM and causing
   setattr_copy() to write INVALID_UID / INVALID_GID into inode->i_uid and
   inode->i_gid.

2. In chown_ids_r() -> get_kuid_r() / get_kgid_r(), malformed
   struct __kernel_uid_r inputs for which make_kuid_r() or make_kgid_r()
   returns INVALID_UID / INVALID_GID pass uid_valid(*kuid) / gid_valid(*kgid)
   and return 0 instead of -EINVAL.

3. In chown_path(), because setattr_vfsuid() and setattr_vfsgid() only return
   false when !uid_valid() or !gid_valid(), gating them with
   uid_valid(ids->uid) and gid_valid(ids->gid) also makes the two
   return -EINVAL branches unreachable.

>  	error = inode_lock_killable(inode);
>  	if (error)

[Severity: High]
What prevents chown_path() from setting a UUID or GUID owner or group on a
non-idmapped mount of a filesystem mounted in a non-initial user namespace
(inode->i_sb->s_user_ns != &init_user_ns)?

When a process in init_user_ns calls chown_r(), lchown_r(), fchown_r(), or
fchownat_r() with a UUID or GUID on a regular mount (idmap == &nop_mnt_idmap)
of a filesystem with SB_I_UID_R mounted in a child user namespace (such as
tmpfs, ramfs, devpts, or overlayfs), chown_path() calls security_path_chown()
with from_vfsuid() / from_vfsgid() and then calls notify_change().

In fs/attr.c:notify_change(), the superblock namespace check uses
vfsuid_has_fsmapping() and vfsgid_has_fsmapping():

    if (ia_valid & ATTR_UID &&
        !vfsuid_has_fsmapping(idmap, inode->i_sb->s_user_ns,
                              attr->ia_vfsuid))
            return -EOVERFLOW;

However, vfsuid_has_fsmapping() in include/linux/mnt_idmapping.h simply
checks uid_valid(from_vfsuid(idmap, fs_userns, vfsuid)), and from_vfsuid() in
fs/mnt_idmapping.c returns early for nop_mnt_idmap before checking
kuid_has_mapping(fs_userns, ...):

    kuid_t from_vfsuid(struct mnt_idmap *idmap,
                       struct user_namespace *fs_userns, vfsuid_t vfsuid)
    {
            uid_t uid;

            if (idmap == &nop_mnt_idmap)
                    return AS_KUIDT(vfsuid);
            if (idmap == &invalid_mnt_idmap)
                    return INVALID_UID;
            if (!vfsuid_is_legacy(vfsuid))
                    return kuid_has_mapping(fs_userns, AS_KUIDT(vfsuid)) ?
                           AS_KUIDT(vfsuid) : INVALID_UID;

Because UUID and GUID identities only have mappings in init_user_ns,
shouldn't non-legacy IDs also be checked against fs_userns when
idmap == &nop_mnt_idmap so that notify_change() rejects them with -EOVERFLOW
instead of writing an unmapped UUID or GUID to the inode?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=21