Re: [RFC PATCH 3/8] KEYS: Reject @desclen values over U16_MAX in key_alloc()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KEYS: desc_len truncation in __key_create_or_update() before key_alloc()
--

commit e49d7d031ada6f60dccb5c6e74d1e66145a0c3d4
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Reject @desclen values over U16_MAX in key_alloc()

This commit adds a bounds check in key_alloc() to reject key descriptions
longer than U16_MAX with -EINVAL, preventing truncation when storing the
length in the 16-bit desc_len field of struct keyring_index_key.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index b34a64d81d47a..a0f083339da8d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -245,6 +245,8 @@ struct key *key_alloc(struct key_type *type, const char *desc,
>  	}
>  
>  	desclen = strlen(desc);
> +	if (desclen > U16_MAX)
> +		goto error;
>  	quotalen = desclen + 1 + type->def_datalen;
>  
>  	/* get hold of the key tracking for this user */

[Severity: High]
Does __key_create_or_update() also need to validate the description length
before assigning index_key.desc_len?

In security/keys/key.c, __key_create_or_update() assigns strlen() to the
16-bit index_key.desc_len field and passes it into __key_link_begin() and
find_key_to_update() before key_alloc() is called:

security/keys/key.c:__key_create_or_update() {
	...
	index_key.desc_len = strlen(index_key.description);
	key_set_index_key(&index_key);

	ret = __key_link_lock(keyring, &index_key);
	if (ret < 0) {
		key_ref = ERR_PTR(ret);
		goto error_free_prep;
	}

	ret = __key_link_begin(keyring, &index_key, &edit);
	if (ret < 0) {
		key_ref = ERR_PTR(ret);
		goto error_link_end;
	}

	...
	if (allow_update) {
		if (index_key.type->update) {
			key_ref = find_key_to_update(keyring, &index_key);
			if (key_ref)
				goto found_matching_key;
		}
	}

	key = key_alloc(index_key.type, index_key.description,
			cred->fsuid, cred->fsgid, cred, perm, flags, NULL);
}

Although add_key() limits user-supplied _description strings to
KEY_MAX_DESC_SIZE, _description can be NULL when index_key.type->preparse
generates prep.description from the payload. For example, in
crypto/asymmetric_keys/x509_public_key.c, x509_key_preparse() constructs
prep->description from the X.509 certificate subject and hex-encoded
serial or skid:

	desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
	if (!desc)
		return -ENOMEM;
	p = memcpy(desc, cert->subject, sulen);
	p += sulen;
	*p++ = ':';
	*p++ = ' ';
	p = bin2hex(p, q, srlen);
	*p = 0;

Since the payload can be up to almost 1MB, strlen(index_key.description)
can exceed U16_MAX.

If strlen(index_key.description) is a multiple of 65536 (or 0),
index_key.desc_len truncates to 0 and triggers BUG_ON(index_key->desc_len == 0)
in __key_link_begin() in security/keys/keyring.c before key_alloc() is
reached.

If strlen(index_key.description) > U16_MAX is not a multiple of 65536,
index_key.desc_len truncates to strlen % 65536. Because
keyring_compare_object() in security/keys/keyring.c only compares
index_key->desc_len bytes:

	return key->index_key.type == index_key->type &&
		key->index_key.domain_tag == index_key->domain_tag &&
		key->index_key.desc_len == index_key->desc_len &&
		memcmp(key->index_key.description, index_key->description,
		       index_key->desc_len) == 0;

find_key_to_update() can match an existing key with a shorter description
sharing that prefix and update the wrong key via goto found_matching_key,
bypassing key_alloc() altogether (or fail with -EEXIST instead of -EINVAL).

In addition, keyring_search() in security/keys/keyring.c, as well as
request_key_and_link() and request_key_rcu() in security/keys/request_key.c,
also assign strlen(description) directly to .index_key.desc_len without
bounds checking.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019142516.300040-1-jarkko@kernel.org?part=3