Re: [RFC PATCH v2 10/41] uidgid: convert between kernel ids and the ids of the *_r system calls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] uidgid: get_kuid_r() and get_kgid_r() accept nonzero reserved bytes
- [Medium] uidgid: get_kuid_r() and get_kgid_r() accept invalid IDs on !CONFIG_MULTIUSER
--

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

uidgid: convert between kernel ids and the ids of the *_r system calls

This commit adds helper functions to convert between kernel kuid_t/kgid_t
values and the struct __kernel_uid_r representation used by the *_r system
calls, along with helpers to copy them to and from userspace and format
them as strings.

> diff --git a/kernel/uidgid_r.c b/kernel/uidgid_r.c
> new file mode 100644
> index 0000000000000..d71aecddb27e4
> --- /dev/null
> +++ b/kernel/uidgid_r.c
> @@ -0,0 +1,323 @@
[ ... ]
> +/*
> + * Whether @id is well formed: a known type, reserved bytes zero, a UUID of
> + * version 4 with the RFC 9562 variant, a GUID other than the nil GUID.
> + */
> +static bool kid_r_valid(const struct __kernel_uid_r *id)
> +{
> +	switch (id->type) {
> +	case UID_R_LEGACY:
> +		return !memchr_inv(id->uuid + sizeof(id->legacy), 0,
> +				   UID_R_SIZE - sizeof(id->legacy));
> +	case UID_R_UUID:
> +		return (id->uuid[6] >> 4) == 4 && (id->uuid[8] & 0xc0) == 0x80;
> +	case UID_R_GUID:
> +		return memchr_inv(id->guid, 0, UID_R_SIZE);
> +	}
> +	return false;
> +}
[ ... ]
> +/*
> + * "No id" from userspace: no pointer, UID_R_NONE, or the classic -1. The
> + * set*id_r() and chown_r() calls take it as "leave unchanged".
> + */
> +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);
> +}

[Severity: Medium]
Does kid_r_is_none() in kernel/uidgid_r.c allow nonzero reserved bytes in
struct __kernel_uid_r to bypass validation in get_kuid_r() and get_kgid_r()?

When userspace passes a struct __kernel_uid_r to get_kuid_r() or
get_kgid_r(), kid_r_is_none(&id) is checked before make_kuid_r() or
make_kgid_r() (which call kid_r_valid(&id)):

	if (kid_r_is_none(&id))
		return 0;
	*kuid = make_kuid_r(from, &id);

If userspace passes id->type == UID_R_NONE with nonzero bytes in id->uuid,
or id->type == UID_R_LEGACY with id->legacy == (u32)-1 and nonzero trailing
bytes in id->uuid + sizeof(id->legacy), kid_r_is_none() returns true and
get_kuid_r() / get_kgid_r() return 0 instead of -EINVAL.

Should kid_r_is_none() also verify that all bytes of id->uuid are zero for
UID_R_NONE and that the trailing reserved bytes are zero for UID_R_LEGACY?

> +
> +/**
> + * get_kuid_r - read a uid of a *_r system call from userspace
> + * @from: user namespace the uid is in
> + * @uid: the uid, or NULL
> + * @kuid: the kuid, INVALID_UID for "no id" (NULL, UID_R_NONE or -1)
> + *
> + * Return: 0, -EFAULT, or -EINVAL if the uid is malformed or has no mapping
> + * in @from.
> + */
> +int get_kuid_r(struct user_namespace *from,
> +	       const struct __kernel_uid_r __user *uid, kuid_t *kuid)
> +{
> +	struct __kernel_uid_r id;
> +
> +	*kuid = INVALID_UID;
> +	if (!uid)
> +		return 0;
> +	if (copy_from_user(&id, uid, sizeof(id)))
> +		return -EFAULT;
> +	if (kid_r_is_none(&id))
> +		return 0;
> +	*kuid = make_kuid_r(from, &id);
> +	return uid_valid(*kuid) ? 0 : -EINVAL;
> +}
> +EXPORT_SYMBOL(get_kuid_r);
> +
> +/**
> + * get_kgid_r - read a gid of a *_r system call from userspace
> + * @from: user namespace the gid is in
> + * @gid: the gid, or NULL
> + * @kgid: the kgid, INVALID_GID for "no id" (NULL, UID_R_NONE or -1)
> + *
> + * Return: 0, -EFAULT, or -EINVAL if the gid is malformed or has no mapping
> + * in @from.
> + */
> +int get_kgid_r(struct user_namespace *from,
> +	       const struct __kernel_uid_r __user *gid, kgid_t *kgid)
> +{
> +	struct __kernel_uid_r id;
> +
> +	*kgid = INVALID_GID;
> +	if (!gid)
> +		return 0;
> +	if (copy_from_user(&id, gid, sizeof(id)))
> +		return -EFAULT;
> +	if (kid_r_is_none(&id))
> +		return 0;
> +	*kgid = make_kgid_r(from, &id);
> +	return gid_valid(*kgid) ? 0 : -EINVAL;
> +}
> +EXPORT_SYMBOL(get_kgid_r);

[Severity: Medium]
What happens in get_kuid_r() and get_kgid_r() in kernel/uidgid_r.c when
CONFIG_MULTIUSER is disabled?

kernel/Makefile builds uidgid_r.o unconditionally via obj-y. When userspace
passes a malformed struct __kernel_uid_r (such as an unknown type, nonzero
reserved bytes, a non-v4 UUID, or a nil GUID), make_kuid_r() and
make_kgid_r() return INVALID_UID and INVALID_GID, and get_kuid_r() and
get_kgid_r() then check uid_valid(*kuid) and gid_valid(*kgid).

In include/linux/uidgid.h, when !CONFIG_MULTIUSER, __kuid_val() and
__kgid_val() unconditionally 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;
}

Because __kuid_val(INVALID_UID) and __kgid_val(INVALID_GID) return 0 under
!CONFIG_MULTIUSER, uid_valid(INVALID_UID) and gid_valid(INVALID_GID) always
evaluate to true. Will this cause get_kuid_r() and get_kgid_r() to return 0
instead of -EINVAL for malformed IDs on !CONFIG_MULTIUSER builds?

[ ... ]

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