Re: [PATCH v2 2/6] 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: integer truncation of desc_len in __key_create_or_update()
--

commit 40e3162313abde79a8374f519fc697c857a50981
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 adds a check
in key_alloc() to reject descriptions with a length at or above
KEY_MAX_DESC_SIZE with -EINVAL so that the 16-bit desc_len field in struct
keyring_index_key does not overflow.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index b34a64d81d47a..4f52245581a4c 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 for the __key_create_or_update()
path?

In security/keys/key.c:__key_create_or_update(), strlen(index_key.description)
is assigned to the 16-bit index_key.desc_len field and passed to
key_set_index_key(), __key_link_begin(), and find_key_to_update() before
key_alloc() is ever called:

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);
	...
}

When userspace calls sys_add_key() with a NULL _description, the
strndup_user(_description, KEY_MAX_DESC_SIZE) check in sys_add_key() is
skipped and index_key.type->preparse() can generate prep.description from the
payload (for example, x509_key_preparse() in
crypto/asymmetric_keys/x509_public_key.c builds prep->description from the
certificate subject and serial/SKID without a KEY_MAX_DESC_SIZE limit).
In-kernel callers of key_create_or_update() and key_create() also pass
description directly into __key_create_or_update().

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 security/keys/keyring.c:__key_link_begin() before key_alloc() is reached.
If strlen(index_key.description) > 65535 truncates to a non-zero 16-bit length
L, key_set_index_key() and find_key_to_update() hash and match only the first
L bytes of index_key.description, which can collide with and update an
existing key with that L-byte prefix without ever reaching key_alloc().

Should __key_create_or_update() also validate that the description length is
non-zero and less than KEY_MAX_DESC_SIZE before assigning index_key.desc_len?

[ ... ]

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