Re: [PATCH 1/3] keys: fix out-of-bounds read in keyring_get_key_chunk()

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sat, Jul 11, 2026 at 09:44:58PM -0400, Michael Bommarito wrote:
> For description-level chunks keyring_get_key_chunk() advances the read
> pointer by level * sizeof(long) past the inline prefix but only
> bounds-checks the prefix, so a long enough key description is read past
> its kmemdup(desc, desc_len + 1) allocation.  Compute the full byte
> offset and bounds-check the description against it before reading.
> 
> The walk only reaches a description-level chunk when two keys collide
> through the hash, x, type and domain_tag chunks, so this is reached from
> an unprivileged add_key(2) with a crafted pair of same-type keys whose
> index hashes collide; KASAN reports a slab-out-of-bounds read.
> 
> Fixes: f771fde82051 ("keys: Simplify key description management")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>  security/keys/keyring.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> KASAN, x86_64: add_key(2) of a crafted hash-colliding "user"-key pair
> (~63-byte descriptions) reports
> 
>   BUG: KASAN: slab-out-of-bounds in keyring_get_key_chunk
>   keyring_get_key_chunk <- assoc_array_insert <- __key_link_begin
>     <- __do_sys_add_key
> 
> reading one byte past the description allocation; the same trigger is
> KASAN-clean with this patch.  On a kernel built without init-on-alloc,
> reading the colliding keyring back with KEYCTL_READ returns
> uninitialized slab until patches 2 and 3 are applied too.  Trigger
> available off-list.
> 
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 7a2ee0ded7c93..1739373172ad5 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -270,7 +270,7 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
>  	const struct keyring_index_key *index_key = data;
>  	unsigned long chunk = 0;
>  	const u8 *d;
> -	int desc_len = index_key->desc_len, n = sizeof(chunk);
> +	int desc_len = index_key->desc_len, n = sizeof(chunk), offset;

Just a nut but this is quite nasty looking statement to begin with, so
at least I would not extend it.

I'd add "int offset" to its own line. Maybe for the sake of clarity it
should be also unsigned int unless negative values hold some merit.

>  
>  	level /= ASSOC_ARRAY_KEY_CHUNK_SIZE;
>  	switch (level) {
> @@ -284,12 +284,12 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
>  		return (unsigned long)index_key->domain_tag;
>  	default:
>  		level -= 4;
> -		if (desc_len <= sizeof(index_key->desc))
> +		offset = sizeof(index_key->desc) + level * sizeof(long);
> +		if (desc_len <= offset)
>  			return 0;
>  
> -		d = index_key->description + sizeof(index_key->desc);
> -		d += level * sizeof(long);
> -		desc_len -= sizeof(index_key->desc);
> +		d = index_key->description + offset;
> +		desc_len -= offset;
>  		if (desc_len > n)
>  			desc_len = n;
>  		do {
> -- 
> 2.53.0
> 

BR, Jarkko