Re: [RFC PATCH v2 12/41] keys: name per-user keyrings of UUID and GUID users by the whole uid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] keys: uid_valid() and gid_valid() break keyctl_chown_kid() on !MULTIUSER
--

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

keys: name per-user keyrings of UUID and GUID users by the whole uid

This commit names per-user and persistent keyrings of UUID and GUID users
using kuid_str() instead of -1, passes the text form to request-key, and
adds KEYCTL_CHOWN_R to support changing key ownership using __kernel_uid_r
identifiers.

> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d14ace88e529c..984c9c54a51ff 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -946,26 +946,17 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>   *
>   * If successful, 0 will be returned.
>   */
> -long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> +static long keyctl_chown_kid(key_serial_t id, kuid_t uid, kgid_t gid)
>  {
>  	struct key_user *newowner, *zapowner = NULL;
>  	struct key *key;
>  	key_ref_t key_ref;
>  	long ret;
> -	kuid_t uid;
> -	kgid_t gid;
>  	unsigned long flags;
>  
> -	uid = make_kuid(current_user_ns(), user);
> -	gid = make_kgid(current_user_ns(), group);
> -	ret = -EINVAL;
> -	if ((user != (uid_t) -1) && !uid_valid(uid))
> -		goto error;
> -	if ((group != (gid_t) -1) && !gid_valid(gid))
> -		goto error;
> -
> +	/* invalid ids leave the owner or group unchanged */
>  	ret = 0;
> -	if (user == (uid_t) -1 && group == (gid_t) -1)
> +	if (!uid_valid(uid) && !gid_valid(gid))
>  		goto error;
>  
>  	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
> @@ -985,12 +976,12 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  		bool is_privileged_op = false;
>  
>  		/* only the sysadmin can chown a key to some other UID */
> -		if (user != (uid_t) -1 && !uid_eq(key->uid, uid))
> +		if (uid_valid(uid) && !uid_eq(key->uid, uid))
>  			is_privileged_op = true;
>  
>  		/* only the sysadmin can set the key's GID to a group other
>  		 * than one of those that the current process subscribes to */
> -		if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid))
> +		if (gid_valid(gid) && !gid_eq(gid, key->gid) && !in_group_p(gid))
>  			is_privileged_op = true;
>  
>  		if (is_privileged_op && !capable(CAP_SYS_ADMIN))
> @@ -998,7 +989,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  	}
>  
>  	/* change the UID */
> -	if (user != (uid_t) -1 && !uid_eq(uid, key->uid)) {
> +	if (uid_valid(uid) && !uid_eq(uid, key->uid)) {
>  		ret = -ENOMEM;
>  		newowner = key_user_lookup(uid);
>  		if (!newowner)
> @@ -1042,7 +1033,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  	}
>  
>  	/* change the GID */
> -	if (group != (gid_t) -1)
> +	if (gid_valid(gid))
>  		key->gid = gid;
>  
>  	notify_key(key, NOTIFY_KEY_SETATTR, 0);
> @@ -1063,6 +1054,39 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  	goto error_put;
>  }
>  
> +long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> +{
> +	kuid_t uid = make_kuid(current_user_ns(), user);
> +	kgid_t gid = make_kgid(current_user_ns(), group);
> +
> +	if ((user != (uid_t) -1) && !uid_valid(uid))
> +		return -EINVAL;
> +	if ((group != (gid_t) -1) && !gid_valid(gid))
> +		return -EINVAL;
> +	return keyctl_chown_kid(id, uid, gid);
> +}
> +
> +/*
> + * KEYCTL_CHOWN with __kernel_uid_r_t ids, which can be UUIDs and GUIDs. A
> + * NULL pointer, UID_R_NONE or a classic -1 leave the owner or group
> + * unchanged.
> + */
> +long keyctl_chown_key_r(key_serial_t id,
> +			const struct __kernel_uid_r __user *user,
> +			const struct __kernel_uid_r __user *group)
> +{
> +	kuid_t uid;
> +	kgid_t gid;
> +	long ret;
> +
> +	ret = get_kuid_r(current_user_ns(), user, &uid);
> +	if (!ret)
> +		ret = get_kgid_r(current_user_ns(), group, &gid);
> +	if (ret)
> +		return ret;
> +	return keyctl_chown_kid(id, uid, gid);
> +}

[Severity: Medium]
Does replacing the explicit (uid_t)-1 and (gid_t)-1 checks in
security/keys/keyctl.c:keyctl_chown_kid() with uid_valid() and gid_valid()
introduce a regression when CONFIG_MULTIUSER is disabled?

In include/linux/uidgid.h, when CONFIG_MULTIUSER is not enabled,
__kuid_val() and __kgid_val() unconditionally return 0:

    #ifdef CONFIG_MULTIUSER
    ...
    #else
    static inline uid_t __kuid_val(kuid_t uid)
    {
            return 0;
    }

    static inline gid_t __kgid_val(kgid_t gid)
    {
            return 0;
    }
    #endif

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

    static inline bool gid_valid(kgid_t gid)
    {
            return !gid_is_legacy(gid) || __kgid_val(gid) != (gid_t) -1;
    }

Because INVALID_UID and INVALID_GID have type KID_LEGACY and __kuid_val() /
__kgid_val() return 0 under !CONFIG_MULTIUSER, uid_valid(INVALID_UID) and
gid_valid(INVALID_GID) always evaluate to true.

When userspace invokes keyctl(KEYCTL_CHOWN) or keyctl(KEYCTL_CHOWN_R) on a
kernel built with CONFIG_KEYS=y and CONFIG_MULTIUSER=n:

1. Calling keyctl_chown_key() with user == -1 and group == -1, or calling
   keyctl_chown_key_r() with NULL, UID_R_NONE, or -1 for both arguments,
   passes INVALID_UID and INVALID_GID into keyctl_chown_kid(). Because
   !uid_valid(uid) && !gid_valid(gid) is false, the no-op early return is
   skipped. It then calls lookup_user_key() with KEY_LOOKUP_CREATE (which
   can allocate a keyring or fail with -ENOKEY or -EACCES), overwrites
   key->gid with INVALID_GID, and posts a spurious NOTIFY_KEY_SETATTR watch
   notification.

2. Calling keyctl_chown_key() or keyctl_chown_key_r() to change only the
   owner while leaving the group unchanged (-1, NULL, or UID_R_NONE) passes
   gid = INVALID_GID into keyctl_chown_kid(), where if (gid_valid(gid))
   evaluates to true and clobbers key->gid with INVALID_GID.

3. In keyctl_chown_key_r() -> get_kuid_r() / get_kgid_r() in
   kernel/uidgid_r.c:

    *kuid = make_kuid_r(from, &id);
    return uid_valid(*kuid) ? 0 : -EINVAL;

   Malformed __kernel_uid_r inputs that cause make_kuid_r() or make_kgid_r()
   to return INVALID_UID or INVALID_GID will pass the uid_valid() /
   gid_valid() check and return 0 instead of -EINVAL.

Would it make sense for uid_valid() and gid_valid() (or __kuid_val() and
__kgid_val()) to preserve the -1 check for INVALID_UID and INVALID_GID when
!CONFIG_MULTIUSER, or to check for unchanged IDs explicitly?

[ ... ]

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