Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()

[sashiko-bot@kernel.org]

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

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

KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()

This commit moves KEY_MAX_DESC_SIZE to include/linux/key.h and rejects
descriptions with length at or above KEY_MAX_DESC_SIZE in key_alloc() to
prevent overflowing the 16-bit desc_len field in struct keyring_index_key.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index a438c4508595d..aa0112a693e68 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 >= KEY_MAX_DESC_SIZE)
> +		goto error;
>  	quotalen = desclen + 1 + type->def_datalen;
>  
>  	/* get hold of the key tracking for this user */

[Severity: Critical]
Is checking desclen in key_alloc() too late when a key is created or
updated through __key_create_or_update()?

In security/keys/key.c, __key_create_or_update() can obtain
index_key.description from index_key.type->preparse(&prep) (for example,
via add_key() with a NULL description and the "asymmetric" key type, where
x509_key_preparse() in crypto/asymmetric_keys/x509_public_key.c constructs
prep.description from the X.509 subject and hex-encoded serial or skid
without bounding it to KEY_MAX_DESC_SIZE) or from an in-kernel caller of
key_create_or_update().

Before calling key_alloc(), __key_create_or_update() assigns the length
directly into the 16-bit index_key.desc_len field and passes index_key to
the keyring lookup and linking helpers:

security/keys/key.c:__key_create_or_update() {
	if (index_key.type->preparse) {
		ret = index_key.type->preparse(&prep);
		if (ret < 0) {
			key_ref = ERR_PTR(ret);
			goto error_free_prep;
		}
		if (!index_key.description)
			index_key.description = prep.description;
		key_ref = ERR_PTR(-EINVAL);
		if (!index_key.description)
			goto error_free_prep;
	}
	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);
	...
	key_ref = find_key_to_update(keyring_ref, &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);
}

If strlen(index_key.description) is 65536 (which x509_key_preparse() can
produce from a ~33 KB certificate payload because bin2hex() doubles the
serial or skid length), truncating it to the u16 index_key.desc_len yields
0 and hits BUG_ON(index_key->desc_len == 0) in __key_link_begin() in
security/keys/keyring.c before key_alloc() is ever reached:

security/keys/keyring.c:__key_link_begin() {
	...
	BUG_ON(index_key->desc_len == 0);
}

Similarly, if strlen(index_key.description) is >= KEY_MAX_DESC_SIZE and not
a multiple of 65536, the unvalidated or truncated desc_len is used by
key_set_index_key(), __key_link_begin(), and find_key_to_update(), which
can match and update an existing key with that shorter prefix via
goto found_matching_key without calling key_alloc(). Other paths that
populate struct keyring_index_key directly, such as keyring_search() in
security/keys/keyring.c and request_key_and_link() / request_key_rcu() in
security/keys/request_key.c, also assign index_key.desc_len =
strlen(description) without checking against KEY_MAX_DESC_SIZE.

Should the KEY_MAX_DESC_SIZE check also be enforced in
__key_create_or_update() (and other struct keyring_index_key initialization
paths) before assigning index_key.desc_len?

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