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

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 28/08/26 14:18, Coiby Xu wrote:
> In case user adds/deletes the keys by mistake, it's safer to read the
> number of keys from reserved memory.
>
> Fixes: 9ebfa8dcaea7 ("crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging")
> Reported-and-Suggested-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   kernel/crash_dump_dm_crypt.c | 36 +++++++++++++++++++++++-------------
>   1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 026c7de4ad85..0b9e09c60745 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -89,21 +89,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);
> +kunmap:
>   	kunmap_local(keys_header_loaded);
>   	arch_kexec_protect_crashkres();
> -
> -	return 0;
> +	return r;
>   }
>   
>   static int restore_dm_crypt_keys_to_thread_keyring(void)
> @@ -447,12 +457,12 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	mutex_lock(&config_keys_subsys.su_mutex);
>   	mutex_acquired = true;
>   
> -	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");
> +			return 0;
> +		}
> +

Not directly related to this patch, but I have a query.

Do we really need to take config_keys_subsys.su_mutex when keys are
reused? If not, how about moving the acquisition and release of
config_keys_subsys.su_mutex into build_keys_header()?


- Sourabh Jain
>   		r = build_keys_header();
>   		if (r)
>   			goto out;
> @@ -463,7 +473,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	 * cleaned up at the end of kexec_file_load syscall
>   	 */
>   	kbuf.buffer = keys_header;
> -	kbuf.bufsz = get_keys_header_size(key_count);
> +	kbuf.bufsz = get_keys_header_size(keys_header->total_keys);
>   
>   	kbuf.memsz = kbuf.bufsz;
>   	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;