Re: [RFC PATCH 5/8] KEYS: Prevent overflow of quota in key_alloc()

[sashiko-bot@kernel.org]

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

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

KEYS: Prevent overflow of quota in key_alloc()

This commit adds a check in key_alloc() to ensure that the calculated
quotalen does not exceed INT_MAX when combining desclen and
type->def_datalen.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index 2996a387a3553..24d1c33ff87b4 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -245,7 +245,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
>  	}
>  
>  	desclen = strlen(desc);
> -	if (desclen > U16_MAX)
> +	if (desclen > U16_MAX || type->def_datalen > INT_MAX - desclen - 1)
>  		goto error;
>  	quotalen = desclen + 1 + type->def_datalen;
>  

[Severity: High]
Is checking desclen > U16_MAX in key_alloc() sufficient to prevent u16
truncation of index_key.desc_len in the __key_create_or_update() path?

In security/keys/key.c:__key_create_or_update(), when index_key.type->preparse
generates prep.description (or a caller passes a long description to
key_create_or_update() or key_create()), strlen(index_key.description) is
assigned directly to the u16 field index_key.desc_len and passed to
__key_link_begin() and find_key_to_update() before key_alloc() is ever
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);
	...
}

For example, when add_key("asymmetric", NULL, payload, plen, keyring) is
called with a crafted X.509 certificate, asn1_ber_decoder() caps the input
blob at datalen <= 65535, but x509_key_preparse() constructs
prep->description by hex-encoding cert->raw_serial (or cert->raw_skid) via
bin2hex(), which doubles its length:

crypto/asymmetric_keys/x509_public_key.c:x509_key_preparse() {
	...
	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;
	...
}

If a certificate has sulen == 0 and srlen == 32767 (with distinct issuer
and subject so x509_check_for_self_signed() skips signature verification),
strlen(prep.description) is 0 + 2 + 32767 * 2 = 65536 (U16_MAX + 1).

When __key_create_or_update() assigns 65536 to u16 index_key.desc_len, it
truncates to 0, and __key_link_begin() immediately hits a BUG_ON before
reaching key_alloc():

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

If strlen(index_key.description) exceeds U16_MAX and is not a multiple of
65536, index_key.desc_len truncates modulo 65536, causing
__key_link_begin() and find_key_to_update() to operate on a truncated
description length before key_alloc() can reject it.

Should __key_create_or_update() also validate strlen(index_key.description)
<= U16_MAX before assigning index_key.desc_len?

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