Re: [PATCH v5 2/9] crash_dump: Read the number of dm-crypt keys from reserved memory

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditionally clearing `is_dm_key_reused` defeats the key caching mechanism, silently breaking hotplug kdump reloads and ARM64 segment retries.

Pre-existing issues:
- [High] Out-of-bounds read past the 4KB window of `kmap_local_page` when reading dm-crypt keys from reserved memory.
--

commit f9061da466f53a4935cffc1bfaa1ac6f1ef0c0ef
Author: Coiby Xu <coiby.xu@gmail.com>

crash_dump: Read the number of dm-crypt keys from reserved memory

This patch reads the number of dm-crypt keys from reserved memory to handle
cases where users add or delete keys by mistake. This aims to improve safety
over reusing saved keys for CPU/memory hot-plugging.

> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index f638fb4b8554c..3a416933979f8 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -88,21 +88,31 @@ static int get_keys_from_kdump_reserved_memory(void)
>  {
>  	struct keys_header *keys_header_loaded;
>  	size_t keys_header_size;
> -
> -	keys_header_size = get_keys_header_size(key_count);
> -	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
> -	if (!keys_header)
> -		return -ENOMEM;
> +	int r = 0;
>  
>  	arch_kexec_unprotect_crashkres();
>  	keys_header_loaded = kmap_local_page(pfn_to_page(
>  		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
>  
> +	if (keys_header_loaded->total_keys <= 0 ||
> +	    keys_header_loaded->total_keys > KEY_NUM_MAX) {
> +		pr_warn("keys_header saved to reserved memory may be corrupt\n");
> +		r = -EINVAL;
> +		goto kunmap;
> +	}
> +
> +	keys_header_size = get_keys_header_size(keys_header_loaded->total_keys);
> +	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
> +	if (!keys_header) {
> +		r = -ENOMEM;
> +		goto kunmap;
> +	}
> +
>  	memcpy(keys_header, keys_header_loaded, keys_header_size);

[Severity: High]
This is a pre-existing issue, but does this memcpy() cause an out-of-bounds
read in get_keys_from_kdump_reserved_memory()?

The pointer keys_header_loaded is set up using kmap_local_page(), which
establishes a virtual mapping strictly limited to a single 4KB page.
However, keys_header_size can reach up to approximately 49.6KB (128 keys
at 388 bytes each).

Could this read past the kmap_local_page window if enough keys are populated
to exceed the 4KB boundary, causing a kernel page fault or copying unrelated
memory into the dm-crypt keys buffer?

> +kunmap:
>  	kunmap_local(keys_header_loaded);
>  	arch_kexec_protect_crashkres();
> -
> -	return 0;
> +	return r;
>  }

[ ... ]

> @@ -432,12 +442,13 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>  	};
>  	int r = 0;
>  
> -	if (key_count <= 0) {
> -		kexec_dprintk("No dm-crypt keys\n");
> -		return 0;
> -	}
> -
>  	if (!is_dm_key_reused) {
> +		if (key_count <= 0) {
> +			kexec_dprintk("No dm-crypt keys\n");
> +			r = 0;
> +			goto out;
> +		}
> +

[Severity: High]
Does this goto out path in crash_load_dm_crypt_keys() unintentionally defeat
the key caching mechanism?

By jumping to the out label where is_dm_key_reused is set to false, it
appears the kexec cleanup function (crash_dm_crypt_cleanup) will now
unconditionally free keys_header since the flag is cleared.

On architectures like ARM64, if kexec segment allocation fails and retries,
the dm-crypt keys may be silently omitted from the new kdump image because
the retry loop will skip loading them (returning success but leaving out the
keys segment).

Could this break the intended caching mechanism for CPU and memory hotplug
events, leading to a silent failure to unlock the encrypted dump target
during a crash?

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