Re: [RFC PATCH v2 09/41] uidgid: let kuid_t and kgid_t hold a UUID or a GUID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] fs: VFSUIDT_INIT_RAW() initializes .type instead of .legacy
- [High] quota: qid_has_mapping() returns false for UUID/GUIDs in init_user_ns
- [High] uidgid: 20-byte kuid_t and kgid_t cause torn reads and writes on inode ids
- [High] uidgid: __kid_cmp() ordering breaks fib_rule_match() for UUID/GUID sockets
- [High] audit: broken audit_field union aliasing in audit_krule_to_data()
- [Medium] uidgid: __kid_hash() mismatches __kid_cmp() when !CONFIG_MULTIUSER
- [Low] uidgid: 20-byte kuid_t and kgid_t break READ_ONCE()/WRITE_ONCE() in tun.c
--

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

uidgid: let kuid_t and kgid_t hold a UUID or a GUID

This commit turns kuid_t, kgid_t, vfsuid_t, and vfsgid_t into 20-byte tagged
unions capable of holding a classic 32-bit ID, an RFC 9562 UUID, or a
Microsoft GUID. It also updates ID comparison, hashing, user namespace
mapping, and mount idmapping helpers to handle non-legacy IDs.

> diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c
> index cb61fbdb52e90..c36e74a37138c 100644
> --- a/fs/mnt_idmapping.c
> +++ b/fs/mnt_idmapping.c
> @@ -87,6 +87,10 @@ vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
>  		return VFSUIDT_INIT(kuid);
>  	if (idmap == &invalid_mnt_idmap)
>  		return INVALID_VFSUID;
> +	/* idmappings map classic ids only; UUIDs and GUIDs pass unchanged */
> +	if (!uid_is_legacy(kuid))
> +		return kuid_has_mapping(fs_userns, kuid) ?
> +		       VFSUIDT_INIT(kuid) : INVALID_VFSUID;
>  	if (initial_idmapping(fs_userns))
>  		uid = __kuid_val(kuid);
>  	else

[Severity: High]
When make_vfsuid() and make_vfsgid() in fs/mnt_idmapping.c map a classic ID on
an idmapped mount, they return via VFSUIDT_INIT_RAW() and VFSGIDT_INIT_RAW():

fs/mnt_idmapping.c:
    #define VFSUIDT_INIT_RAW(val) (vfsuid_t){ val }
    #define VFSGIDT_INIT_RAW(val) (vfsgid_t){ val }
    ...
    return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));

Now that vfsuid_t and vfsgid_t in include/linux/mnt_idmapping.h have u32 type
as their first struct member, doesn't this positional initializer assign the
mapped 32-bit UID or GID to .type while zero-initializing .legacy and .uuid?

When VFS callers on an idmapped mount map an inode's classic ID through
i_uid_into_vfsuid() or i_gid_into_vfsgid() (such as acl_permission_check(),
inode_owner_or_capable(), may_follow_link(), chown_ok(), and
generic_fillattr()):

- Mapped ID 1 sets .type = KID_UUID with an all-zero UUID, and mapped ID 2
  sets .type = KID_GUID with an all-zero GUID, allowing a caller with an
  all-zero UUID or GUID credential to match files owned by mapped UID 1 or 2
  in vfsuid_eq_kuid().

- Other non-zero mapped IDs set .type to an unknown tag with .legacy = 0, so
  vfsuid_is_legacy() returns false and __vfsuid_val() returns (uid_t)-1.

- When map_id_down() returns (uid_t)-1 for an unmapped ID,
  VFSUIDT_INIT_RAW((uid_t)-1) sets .type = (u32)-1, causing vfsuid_valid() and
  vfsgid_valid() in include/linux/mnt_idmapping.h:

    static inline bool vfsuid_valid(vfsuid_t uid)
    {
    	return !vfsuid_is_legacy(uid) || __vfsuid_val(uid) != (uid_t)-1;
    }

  to see !vfsuid_is_legacy(uid) as true and treat unmapped IDs as valid.

Should VFSUIDT_INIT_RAW() and VFSGIDT_INIT_RAW() initialize
.type = KID_LEGACY and .legacy = (val) like KUIDT_INIT() and KGIDT_INIT()?

[ ... ]

> diff --git a/include/linux/uidgid.h b/include/linux/uidgid.h
> index 182c682458cb6..e1adb50d5dac5 100644
> --- a/include/linux/uidgid.h
> +++ b/include/linux/uidgid.h

[ ... ]

> @@ -64,64 +69,115 @@ static inline gid_t __kgid_val(kgid_t gid)
>  #define INVALID_UID KUIDT_INIT(-1)
>  #define INVALID_GID KGIDT_INIT(-1)
>  
> +/*
> + * Ids are ordered by kind first (legacy ids before UUIDs before GUIDs),
> + * then by value: numerically for legacy ids, bytewise for the others.
> + * That keeps sorted id arrays and range checks between legacy ids
> + * working: a UUID is never inside a range of legacy ids.
> + */
> +static inline int __kid_cmp(u32 ltype, u32 llegacy, const u8 *lb,
> +			    u32 rtype, u32 rlegacy, const u8 *rb)
> +{
> +	if (!IS_ENABLED(CONFIG_MULTIUSER))
> +		return 0;
> +	if (ltype != rtype)
> +		return ltype < rtype ? -1 : 1;
> +	if (ltype == KID_LEGACY)
> +		return llegacy == rlegacy ? 0 : llegacy < rlegacy ? -1 : 1;
> +	return __builtin_memcmp(lb, rb, UUID_SIZE);
> +}

[Severity: High]
How does this ordering interact with fib_rule_match() in net/core/fib_rules.c
when a socket is owned by a KID_UUID or KID_GUID user?

In net/core/fib_rules.c, FIB rules without an explicit FRA_UID_RANGE attribute
(including the default local, main, and default table rules added by
fib_default_rule_add() and rules created in fib_nl2rule()) initialize
rule->uid_range to fib_kuid_range_unset:

net/core/fib_rules.c:
    static const struct fib_kuid_range fib_kuid_range_unset = {
    	KUIDT_INIT(0),
    	KUIDT_INIT(~0),
    };

When policy routing is active and fib_rules_lookup() calls fib_rule_match(),
it checks fl->flowi_uid (populated from sk_uid(sk) in paths such as
build_sk_flow_key() in net/ipv4/route.c and ip6_route_output_flags() in
net/ipv6/route.c) unconditionally against rule->uid_range without checking
uid_range_set(&rule->uid_range):

net/core/fib_rules.c:fib_rule_match()
    if (uid_lt(fl->flowi_uid, rule->uid_range.start) ||
        uid_gt(fl->flowi_uid, rule->uid_range.end))
    	goto out;

Because __kid_cmp() orders KID_UUID and KID_GUID after all KID_LEGACY values,
uid_gt(fl->flowi_uid, fib_kuid_range_unset.end) will evaluate to true for any
UUID or GUID socket, rejecting all unset-UID rules (including the default
rules) and failing route lookups with -ESRCH (-ENETUNREACH).

[ ... ]

> +/* A hash of an id, for hash tables keyed by kuid_t or kgid_t */
> +static inline u32 __kid_hash(u32 type, u32 legacy, const u8 *b)
> +{
> +	u32 w[4];
> +
> +	if (type == KID_LEGACY)
> +		return legacy;
> +	__builtin_memcpy(w, b, sizeof(w));
> +	return w[0] ^ w[1] ^ w[2] ^ w[3] ^ type;
> +}
> +
> +static inline u32 kuid_hash(kuid_t uid)
> +{
> +	return __kid_hash(uid.type, __kuid_val(uid), uid.uuid.b);
> +}

[Severity: Medium]
When CONFIG_MULTIUSER is disabled, __kid_cmp() returns 0 unconditionally so
uid_eq() treats all kuid_t values as equal regardless of .type, whereas
__kid_hash() still hashes the UUID/GUID bytes when type != KID_LEGACY.

On a !CONFIG_MULTIUSER kernel, if an inode with a KID_UUID or KID_GUID owner
(e.g. from a mounted ext4 uid_r filesystem or via chown_r() / fchown_r() in
fs/open.c) and S_ISUID set is executed via execve(), bprm_fill_uid() in
fs/exec.c sets bprm->cred->euid to that UUID or GUID. When inc_ucount() ->
alloc_ucounts() -> find_ucounts() in kernel/ucount.c (or uidhashentry() in
kernel/user.c, or later fs/ecryptfs/messaging.c) hashes that kuid_t with
kuid_hash(), won't it land in a non-zero bucket, miss init_ucounts (hashed at
GLOBAL_ROOT_UID), and allocate duplicate ucounts structures even though
uid_eq(init_ucounts.uid, uid) is true?

Should __kid_hash() also return 0 when !IS_ENABLED(CONFIG_MULTIUSER)?

> +
> +static inline u32 kgid_hash(kgid_t gid)
> +{
> +	return __kid_hash(gid.type, __kgid_val(gid), gid.uuid.b);
> +}
>  
>  #ifdef CONFIG_USER_NS
> @@ -134,13 +190,22 @@ extern gid_t from_kgid(struct user_namespace *to, kgid_t gid);
>  extern uid_t from_kuid_munged(struct user_namespace *to, kuid_t uid);
>  extern gid_t from_kgid_munged(struct user_namespace *to, kgid_t gid);
>  
> +/*
> + * UUIDs and GUIDs aren't in any namespace's uid_map or gid_map: they
> + * exist in the initial user namespace only, and nobody has privilege
> + * over them through a capability in another one.
> + */
>  static inline bool kuid_has_mapping(struct user_namespace *ns, kuid_t uid)
>  {
> +	if (!uid_is_legacy(uid))
> +		return ns == &init_user_ns;
>  	return from_kuid(ns, uid) != (uid_t) -1;
>  }
>  
>  static inline bool kgid_has_mapping(struct user_namespace *ns, kgid_t gid)
>  {
> +	if (!gid_is_legacy(gid))
> +		return ns == &init_user_ns;
>  	return from_kgid(ns, gid) != (gid_t) -1;
>  }

[Severity: High]
kuid_has_mapping() and kgid_has_mapping() (along with vfsuid_has_mapping() and
vfsgid_has_mapping() in include/linux/mnt_idmapping.h) are updated here
because from_kuid() and from_kgid() in kernel/user_namespace.c now return
(uid_t)-1 and (gid_t)-1 for UUID and GUID IDs even in init_user_ns.

However, qid_has_mapping() in include/linux/quota.h still checks:

include/linux/quota.h:
    static inline bool qid_has_mapping(struct user_namespace *ns, struct kqid qid)
    {
    	return from_kqid(ns, qid) != (qid_t) -1;
    }

Because from_kqid() in fs/quota/kqid.c delegates to from_kuid() and
from_kgid(), qid_has_mapping(&init_user_ns, qid) now returns false for all
UUID and GUID kqid values.

Although the follow-up commit "fs: keep UUID and GUID owners off filesystems
that can't store them" restricts UUID and GUID owners to SB_I_UID_R
filesystems, tmpfs with CONFIG_TMPFS_QUOTA (enabled in "shmem, ramfs: allow
UUID and GUID owners") and ext4 (enabled in "ext4: add the uid_r feature for
UUID and GUID owners") both support VFS quotas and call dquot_initialize() and
dquot_transfer(). Furthermore, while "quota: don't account UUID and GUID
owners" adds a !qid_is_legacy(qid) check returning ERR_PTR(-ESRCH) in dqget()
in fs/quota/dquot.c, it places that check after:

fs/quota/dquot.c:dqget()
    if (!qid_has_mapping(sb->s_user_ns, qid))
    	return ERR_PTR(-EINVAL);

without updating qid_has_mapping(). Doesn't this make the !qid_is_legacy(qid)
check unreachable and cause dqget() to return -EINVAL instead of -ESRCH,
breaking file creation (__ext4_new_inode(), shmem_get_inode()) and ownership
changes (ext4_setattr(), shmem_setattr()) for UUID and GUID owners whenever
quotas are active?

> diff --git a/include/linux/uidgid_types.h b/include/linux/uidgid_types.h
> index b35ac4955a334..0f595d075b4a2 100644
> --- a/include/linux/uidgid_types.h
> +++ b/include/linux/uidgid_types.h
> @@ -3,13 +3,41 @@
>  #define _LINUX_UIDGID_TYPES_H
>  
>  #include <linux/types.h>
> +#include <linux/uuid_types.h>
>  
> +/*
> + * Kinds of kuid_t and kgid_t. KID_LEGACY is 0, so zeroed memory still
> + * holds GLOBAL_ROOT_UID and GLOBAL_ROOT_GID as it did when the ids were
> + * plain numbers. Userspace has its own numbering (UID_R_* in
> + * <uapi/linux/uidgid_r.h>), in which zero is "no id".
> + */
> +enum kid_type {
> +	KID_LEGACY = 0,		/* classic 32 bit id in .legacy */
> +	KID_UUID,		/* RFC 9562 UUID in .uuid */
> +	KID_GUID,		/* Microsoft GUID in .guid */
> +};
> +
> +/*
> + * Only the member .type selects is meaningful; the bytes of .uuid past
> + * .legacy are undefined for a KID_LEGACY id. Copy the whole struct and
> + * use the helpers of <linux/uidgid.h> to look at it.
> + */
>  typedef struct {
> -	uid_t val;
> +	u32 type;
> +	union {
> +		uid_t legacy;
> +		uuid_t uuid;
> +		guid_t guid;
> +	};
>  } kuid_t;

[Severity: High]
Placing u32 type at offset 0 and uid_t legacy at offset 4 of kuid_t and kgid_t
changes the layout of the anonymous union in struct audit_field in
include/linux/audit.h:

include/linux/audit.h:
    struct audit_field {
    	u32				type;
    	union {
    		u32			val;
    		kuid_t			uid;
    		kgid_t			gid;
    		struct {
    			char		*lsm_str;
    			void		*lsm_rule;
    		};
    	};
    	u32				op;
    };

In kernel/auditfilter.c, audit_data_to_entry() populates f->uid = make_kuid()
or f->gid = make_kgid() for UID and GID filter fields (AUDIT_LOGINUID,
AUDIT_UID, AUDIT_EUID, AUDIT_SUID, AUDIT_FSUID, AUDIT_OBJ_UID, AUDIT_GID,
AUDIT_EGID, AUDIT_SGID, AUDIT_FSGID, AUDIT_OBJ_GID), which now writes
KID_LEGACY (0) at offset 0 and the numeric ID at offset 4.

When audit_krule_to_data() in kernel/auditfilter.c serializes rules back to
userspace for AUDIT_LIST_RULES:

kernel/auditfilter.c:audit_krule_to_data()
    default:
    	data->values[i] = f->val;
    }

none of those UID or GID fields have explicit cases in the switch statement,
so they all read f->val at offset 0 (f->uid.type == 0). Won't this cause
AUDIT_LIST_RULES to report 0 (root) for every UID and GID filter rule?

>  
>  typedef struct {
> -	gid_t val;
> +	u32 type;
> +	union {
> +		gid_t legacy;
> +		uuid_t uuid;
> +		guid_t guid;
> +	};
>  } kgid_t;

[Severity: High]
With kuid_t and kgid_t growing from 4 bytes to 20 bytes, reads and writes of
inode->i_uid and inode->i_gid are no longer single-word atomic and compile
into multi-instruction memory copies.

In the VFS, writers update inode->i_uid and inode->i_gid in place via
notify_change() -> setattr_copy() -> i_uid_update() / i_gid_update() in
include/linux/fs.h (holding only inode->i_rwsem), and via lookup_fast() ->
pid_revalidate() -> pid_update_inode() -> task_dump_owner() in fs/proc/base.c
(under rcu_read_lock() with no inode lock held):

include/linux/fs.h:
    static inline void i_uid_update(struct mnt_idmap *idmap,
    				const struct iattr *attr,
    				struct inode *inode)
    {
    	if (attr->ia_valid & ATTR_UID)
    		inode->i_uid = from_vfsuid(idmap, i_user_ns(inode),
    					   attr->ia_vfsuid);
    }

Meanwhile, lockless readers in acl_permission_check(), may_follow_link(),
may_create_in_sticky(), inode_owner_or_capable(), and generic_fillattr() copy
inode->i_uid and inode->i_gid by value through i_uid_into_vfsuid() and
i_gid_into_vfsgid() without holding inode->i_rwsem or any seqlock:

include/linux/fs.h:
    static inline vfsuid_t i_uid_into_vfsuid(struct mnt_idmap *idmap,
    					 const struct inode *inode)
    {
    	return make_vfsuid(idmap, i_user_ns(inode), inode->i_uid);
    }

Can a concurrent reader racing with chown() / chown_r() or procfs task
revalidation observe a torn 20-byte ID—for example, .type == KID_LEGACY paired
with the first 4 bytes of a UUID/GUID in .legacy (which can match
GLOBAL_ROOT_UID or another local UID), or .type == KID_UUID / KID_GUID paired
with stale union bytes—or can concurrent pid_revalidate() calls leave a
/proc/<pid> inode's ownership permanently torn?

[Severity: Low]
Also, growing kuid_t and kgid_t to 20 bytes causes READ_ONCE() and
WRITE_ONCE() on kuid_t and kgid_t to fail compiletime_assert_rwonce_type() in
include/asm-generic/rwonce.h.

Unlike sk->sk_uid (which is converted to a seqcount in the later commit
"net: sock: read sk_uid through a seqcount"), drivers/net/tun.c still uses
READ_ONCE() and WRITE_ONCE() on tun->owner and tun->group in tun_fill_info()
and __tun_chr_ioctl():

drivers/net/tun.c:tun_fill_info()
    kuid_t owner = READ_ONCE(tun->owner);
    kgid_t group = READ_ONCE(tun->group);

drivers/net/tun.c:__tun_chr_ioctl()
    WRITE_ONCE(tun->owner, owner);
    ...
    WRITE_ONCE(tun->group, group);

Won't this break the build when CONFIG_TUN is enabled?

[ ... ]

> -- 
> 2.51.2

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