Re: [PATCH v5 7/9] crash_dump: Check the function return codes in restore_dm_crypt_keys_to_thread_keyring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Medium] Memory and reference leak of the key returned by key_create_or_update() on the success path.
- [Medium] Incorrect return value assignment from `dm_crypt_keys_read()` breaks the caller's success check for empty key sets.

Pre-existing issues:
- [High] Missing validation of `dm_crypt_keys_addr` leads to arbitrary memory reads.
- [High] Buffer over-read from single mapped page in get_keys_from_kdump_reserved_memory().
- [High] Missing validation on parsed key attributes leads to massive Heap Out-of-Bounds reads and Information Leak.
- [Medium] Double fetch of the key count from untrusted memory leads to Heap Out-of-Bounds read.
--

commit e9dcb18898d315e8ec7102932e9391f9c0892b97
Author: Coiby Xu <coiby.xu@gmail.com>
Subject: crash_dump: Check the function return codes in restore_dm_crypt_keys_to_thread_keyring

This commit adds return code checking for dm_crypt_keys_read() and
add_key_to_keyring() to properly abort when reading old memory or
allocating a keyring fails. It also deletes unused code related to
the key reference.

> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index b8fe630462cfa..bc70fbb79e0f6 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -66,7 +66,7 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
>  			      key_ref_t keyring_ref)
>  {
>  	key_ref_t key_ref;
> -	int r;
> +	int r = 0;
>  
>  	/* create or update the requested key and add it to the target keyring */
>  	key_ref = key_create_or_update(keyring_ref, "user", dm_key->key_desc,
> @@ -74,8 +74,6 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
>  				       KEY_USR_ALL, KEY_ALLOC_IN_QUOTA);
>  
>  	if (!IS_ERR(key_ref)) {
> -		r = key_ref_to_ptr(key_ref)->serial;
> -		key_ref_put(key_ref);
>  		pr_debug("Success adding key %s\n", dm_key->key_desc);

[Severity: Medium]
Does removing key_ref_put() here leak the key reference?

key_create_or_update() returns a key reference specifically for the caller
with an incremented usage count, so without key_ref_put(), the key's usage
count might be permanently elevated, preventing it from ever being garbage
collected.

>  	} else {
>  		r = PTR_ERR(key_ref);

[ ... ]

> @@ -134,9 +132,14 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
>  	}
>  
>  	addr = dm_crypt_keys_addr;
> -	dm_crypt_keys_read((char *)&key_count, sizeof(key_count), &addr);
> +	ret = dm_crypt_keys_read((char *)&key_count, sizeof(key_count), &addr);

[Severity: High]
This is a pre-existing issue, but is it safe to read from dm_crypt_keys_addr
without verifying if it was ever initialized by the command-line argument?

If the kdump kernel is booted without the dmcryptkeys= parameter,
dm_crypt_keys_addr might remain 0, leading to arbitrary memory reads
when this sysfs attribute is triggered.

> +	if (ret < 0) {
> +		pr_err("Failed to read the number of dm-crypt keys\n");
> +		goto out;
> +	}
> +

[ ... ]

> @@ -151,12 +154,18 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
>  		goto out;
>  	}
>  
> -	dm_crypt_keys_read((char *)keys_header, keys_header_size, &addr);
> +	ret = dm_crypt_keys_read((char *)keys_header, keys_header_size, &addr);

[Severity: Medium]
If dm_crypt_keys_read() succeeds, it returns a positive byte count which is
assigned to ret. If there are no keys and the loop below doesn't execute,
will this function incorrectly return a positive value instead of 0?

The caller config_keys_restore_store() expects !ret to set restore = true,
which would fail if ret is a positive byte count.

> +	if (ret < 0) {
> +		pr_err("Failed to read dm-crypt keys\n");
> +		goto out;
> +	}
>  
>  	for (int i = 0; i < keys_header->total_keys; i++) {

[Severity: Medium]
This is a pre-existing issue, but does the second read from oldmem physically
overwrite keys_header->total_keys with unvalidated data?

If oldmem changes concurrently or the read partially fails, the newly fetched
total_keys can exceed the originally validated key_count. The loop bounds
check uses this unvalidated bound, potentially causing a heap out-of-bounds
read.

>  		key = &keys_header->keys[i];
>  		pr_debug("Get key (size=%u)\n", key->key_size);
> -		add_key_to_keyring(key, keyring_ref);
> +		ret = add_key_to_keyring(key, keyring_ref);

[Severity: High]
This isn't a bug introduced by this patch, but are key->key_size and
key->key_desc validated before being passed to add_key_to_keyring()?

The completely untrusted key_size is passed as the payload length to
key_create_or_update(), which might read far out-of-bounds from dm_key->data
and embed leaked kernel heap memory into the key payload. Additionally,
key_desc is not checked for null-termination.

> +		if (ret)
> +			break;
>  	}

[ ... ]

While reviewing this file, I also noticed another pre-existing issue in
get_keys_from_kdump_reserved_memory():

kernel/crash_dump_dm_crypt.c:get_keys_from_kdump_reserved_memory() {
    ...
	keys_header_loaded = kmap_local_page(pfn_to_page(
		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
    ...
	memcpy(keys_header, keys_header_loaded, keys_header_size);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this memcpy() read
past the boundaries of the mapped page?

kmap_local_page() establishes a mapping for exactly one page (4096 bytes),
but keys_header_size could be up to ~49KB. This will read far beyond the
single page boundary, potentially causing a kernel panic on architectures
where kmap_local_page() does not fall back to the direct map.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909003657.1570544-1-coiby.xu@gmail.com?part=7