Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header

Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header

[Sourabh Jain]

Hello Coiby,

On 02/05/26 05:13, Coiby Xu wrote:
> If kexec_add_buffer somehow fails, keys_header will be freed. Depending
> on /sys/kernel/config/crash_dm_crypt_key/reuse, it will lead to the
> following two problems if the kexec_file_load syscall is called again,
>    1. Double free of keys_header if reuse=false
>    2. UAF of keys_header if reuse=true
>
> To address these problems and also make it easier to reason about the
> code, keep two invariants,
>    1. keys_header will always be freed at the end of kexec_file_load
>       syscall except during kdump image unloading for CPU/memory
>       hot-plugging support
>    2. There will always be valid keys_header if reuse=true
>
> Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump reserved memory")
> Fixes: 9ebfa8dcaea7 ("crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging")
> Reported-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   include/linux/kexec.h        |  6 ++++
>   kernel/crash_dump_dm_crypt.c | 65 ++++++++++++++++++++++++++----------
>   kernel/kexec_file.c          |  2 ++
>   3 files changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 8a22bc9b8c6c..91256d7ff434 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -552,6 +552,12 @@ void set_kexec_sig_enforced(void);
>   static inline void set_kexec_sig_enforced(void) {}
>   #endif
>   
> +#ifdef CONFIG_CRASH_DM_CRYPT
> +void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image);
> +#else
> +static inline void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image) {}
> +#endif
> +
>   #endif /* !defined(__ASSEBMLY__) */
>   
>   #endif /* LINUX_KEXEC_H */
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index eac4f436a8d4..4d8a3331bbe7 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -84,18 +84,25 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
>   	return r;
>   }
>   
> -static void get_keys_from_kdump_reserved_memory(void)
> +static int get_keys_from_kdump_reserved_memory(void)
>   {
>   	struct keys_header *keys_header_loaded;
> +	size_t keys_header_size;
>   
> -	arch_kexec_unprotect_crashkres();
> +	keys_header_size = get_keys_header_size(key_count);
> +	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
> +	if (!keys_header)
> +		return -ENOMEM;
>   
> +	arch_kexec_unprotect_crashkres();
>   	keys_header_loaded = kmap_local_page(pfn_to_page(
>   		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));

Accessing kexec_crash_image without holding the kexec lock could lead to
undefined behavior.

I think this section of code should be called by holding kexec lock.


>   
> -	memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
> +	memcpy(keys_header, keys_header_loaded, keys_header_size);
>   	kunmap_local(keys_header_loaded);
>   	arch_kexec_protect_crashkres();
> +
> +	return 0;
>   }
>   
>   static int restore_dm_crypt_keys_to_thread_keyring(void)
> @@ -283,17 +290,28 @@ static ssize_t config_keys_reuse_show(struct config_item *item, char *page)
>   static ssize_t config_keys_reuse_store(struct config_item *item,
>   					   const char *page, size_t count)
>   {
> +	bool val;
> +	int r;
> +
>   	if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {

The above check should be performed after acquiring the kexec lock.



>   		kexec_dprintk(
>   			"dm-crypt keys haven't be saved to crash-reserved memory\n");
>   		return -EINVAL;
>   	}
>   
> -	if (kstrtobool(page, &is_dm_key_reused))
> +	if (kstrtobool(page, &val) || !val)
>   		return -EINVAL;

Why can’t we allow the user to set is_dm_key_reused = false and free the
key_header? That way, the next kdump kernel load will recreate the 
key_header.

For example, a user may add more keys and want the new keys to be included
in the kdump image from next kdump kernel load.


>   
> -	if (is_dm_key_reused)
> -		get_keys_from_kdump_reserved_memory();
> +	if (is_dm_key_reused) {
> +		pr_info("Already got dm-crypt keys, please continue with kexec_file_load syscall\n");
> +	} else {
> +		r = get_keys_from_kdump_reserved_memory();
> +		if (r) {
> +			pr_warn("Failed to get dm-crypt keys from reserved memory\n");
> +			return r;
> +		}
> +		is_dm_key_reused = true;
> +	}
>   
>   	return count;
>   }
> @@ -366,9 +384,6 @@ static int build_keys_header(void)
>   	struct config_key *key;
>   	int i, r;
>   
> -	if (keys_header != NULL)
> -		kvfree(keys_header);
> -
>   	keys_header = kzalloc(get_keys_header_size(key_count), GFP_KERNEL);
>   	if (!keys_header)
>   		return -ENOMEM;
> @@ -412,7 +427,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   		.top_down = false,
>   		.random = true,
>   	};
> -	int r;
> +	int r = 0;
>   
>   
>   	if (key_count <= 0) {
> @@ -421,14 +436,15 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	}
>   
>   	if (!is_dm_key_reused) {
> -		image->dm_crypt_keys_addr = 0;
>   		r = build_keys_header();
> -		if (r) {
> -			pr_err("Failed to build dm-crypt keys header, ret=%d\n", r);
> -			return r;
> -		}
> +		if (r)
> +			goto out;
>   	}
>   
> +	/*
> +	 * keys_header will be copied to reserver memory later and then be
> +	 * cleaned up at the end of kexec_file_load syscall
> +	 */
>   	kbuf.buffer = keys_header;
>   	kbuf.bufsz = get_keys_header_size(key_count);
>   
> @@ -438,18 +454,33 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	r = kexec_add_buffer(&kbuf);
>   	if (r) {
>   		pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
> -		kvfree((void *)kbuf.buffer);
> -		return r;
> +		goto out;
>   	}
> +
>   	image->dm_crypt_keys_addr = kbuf.mem;
>   	image->dm_crypt_keys_sz = kbuf.bufsz;
>   	kexec_dprintk(
>   		"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
>   		kbuf.bufsz, kbuf.memsz);
>   
> +out:
> +	is_dm_key_reused = false;
>   	return r;
>   }
>   
> +void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
> +{
> +	/*
> +	 * For CPU/memory hot-plugging, the kdump image will be reloaded. Prevent
> +	 * keys_header from being cleaned up during unloading when
> +	 * is_dm_key_reused=true
> +	 */
> +	if (!is_dm_key_reused) {
> +		kfree_sensitive(keys_header);
> +		keys_header = NULL;

Since crash_load_dm_crypt_keys() sets is_dm_key_reused = false, 
keys_header will
always be released here, right? Then why is the above free under an if 
condition?

IIUC, for the case where CONFIG_CRASH_HOTPLUG is not enabled, this is 
how key
restore works:

After loading the kdump kernel for the first time, the state of the 
variables is:

is_dm_key_reused = false
keys_header = NULL

For example, if 2 CPUs are hot-removed and kdump is reloaded twice:

Then the sequence of operations needed to ensure the loaded keys can be 
reused is:

Udev rule triggered on the 1st CPU hotplug:
echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
Restore the key header from the reserved area
Reload the kdump service/kernel

Udev rule triggered on the 2nd CPU hotplug:
echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
Restore the key header from the reserved area
Reload the kdump service/kernel

What I don’t understand is the need to restore the key header from 
crashkernel
memory for every hotplug operation.


- Sourabh Jain

> +	}
> +}
> +
>   static int __init configfs_dmcrypt_keys_init(void)
>   {
>   	int ret;
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 2bfbb2d144e6..0421f1e89791 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -139,6 +139,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>   	kfree(image->image_loader_data);
>   	image->image_loader_data = NULL;
>   
> +	kexec_file_post_load_cleanup_dm_crypt(image);
> +
>   	kexec_file_dbg_print = false;
>   }
>   

Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header

[Coiby Xu]

On Wed, May 06, 2026 at 05:58:31PM +0530, Sourabh Jain wrote:
>Hello Coiby,

Hi Sourabh,

Thanks for reviewing the patch!

>
>On 02/05/26 05:13, Coiby Xu wrote:
>>If kexec_add_buffer somehow fails, keys_header will be freed. Depending
>>on /sys/kernel/config/crash_dm_crypt_key/reuse, it will lead to the
>>following two problems if the kexec_file_load syscall is called again,
>>   1. Double free of keys_header if reuse=false
>>   2. UAF of keys_header if reuse=true
>>
>>To address these problems and also make it easier to reason about the
>>code, keep two invariants,
>>   1. keys_header will always be freed at the end of kexec_file_load
>>      syscall except during kdump image unloading for CPU/memory
>>      hot-plugging support
>>   2. There will always be valid keys_header if reuse=true
>>
>>Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump reserved memory")
>>Fixes: 9ebfa8dcaea7 ("crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging")
>>Reported-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>>Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>>---
>>  include/linux/kexec.h        |  6 ++++
>>  kernel/crash_dump_dm_crypt.c | 65 ++++++++++++++++++++++++++----------
>>  kernel/kexec_file.c          |  2 ++
>>  3 files changed, 56 insertions(+), 17 deletions(-)
>>
>>diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>>index 8a22bc9b8c6c..91256d7ff434 100644
>>--- a/include/linux/kexec.h
>>+++ b/include/linux/kexec.h
>>@@ -552,6 +552,12 @@ void set_kexec_sig_enforced(void);
>>  static inline void set_kexec_sig_enforced(void) {}
>>  #endif
>>+#ifdef CONFIG_CRASH_DM_CRYPT
>>+void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image);
>>+#else
>>+static inline void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image) {}
>>+#endif
>>+
>>  #endif /* !defined(__ASSEBMLY__) */
>>  #endif /* LINUX_KEXEC_H */
>>diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>>index eac4f436a8d4..4d8a3331bbe7 100644
>>--- a/kernel/crash_dump_dm_crypt.c
>>+++ b/kernel/crash_dump_dm_crypt.c
>>@@ -84,18 +84,25 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
>>  	return r;
>>  }
>>-static void get_keys_from_kdump_reserved_memory(void)
>>+static int get_keys_from_kdump_reserved_memory(void)
>>  {
>>  	struct keys_header *keys_header_loaded;
>>+	size_t keys_header_size;
>>-	arch_kexec_unprotect_crashkres();
>>+	keys_header_size = get_keys_header_size(key_count);
>>+	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
>>+	if (!keys_header)
>>+		return -ENOMEM;
>>+	arch_kexec_unprotect_crashkres();
>>  	keys_header_loaded = kmap_local_page(pfn_to_page(
>>  		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
>
>Accessing kexec_crash_image without holding the kexec lock could lead to
>undefined behavior.
>
>I think this section of code should be called by holding kexec lock.
>
>
>>-	memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
>>+	memcpy(keys_header, keys_header_loaded, keys_header_size);
>>  	kunmap_local(keys_header_loaded);
>>  	arch_kexec_protect_crashkres();
>>+
>>+	return 0;
>>  }
>>  static int restore_dm_crypt_keys_to_thread_keyring(void)
>>@@ -283,17 +290,28 @@ static ssize_t config_keys_reuse_show(struct config_item *item, char *page)
>>  static ssize_t config_keys_reuse_store(struct config_item *item,
>>  					   const char *page, size_t count)
>>  {
>>+	bool val;
>>+	int r;
>>+
>>  	if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {
>
>The above check should be performed after acquiring the kexec lock.

Good suggestion! I'll try to hold the kexec lock in next version.

>
>
>
>>  		kexec_dprintk(
>>  			"dm-crypt keys haven't be saved to crash-reserved memory\n");
>>  		return -EINVAL;
>>  	}
>>-	if (kstrtobool(page, &is_dm_key_reused))
>>+	if (kstrtobool(page, &val) || !val)
>>  		return -EINVAL;
>
>Why can’t we allow the user to set is_dm_key_reused = false and free the
>key_header? That way, the next kdump kernel load will recreate the 
>
>For example, a user may add more keys and want the new keys to be included
>in the kdump image from next kdump kernel load.

Personally, I want to limit the reuse configfs API to the case of
CPU/memory hotplug thus to make the code simpler. And for the case of a
user adding more keys, I don't think there is a need for using the reuse
API.

>
>
>>-	if (is_dm_key_reused)
>>-		get_keys_from_kdump_reserved_memory();
>>+	if (is_dm_key_reused) {
>>+		pr_info("Already got dm-crypt keys, please continue with kexec_file_load syscall\n");
>>+	} else {
>>+		r = get_keys_from_kdump_reserved_memory();
>>+		if (r) {
>>+			pr_warn("Failed to get dm-crypt keys from reserved memory\n");
>>+			return r;
>>+		}
>>+		is_dm_key_reused = true;
>>+	}
>>  	return count;
>>  }
>>@@ -366,9 +384,6 @@ static int build_keys_header(void)
>>  	struct config_key *key;
>>  	int i, r;
>>-	if (keys_header != NULL)
>>-		kvfree(keys_header);
>>-
>>  	keys_header = kzalloc(get_keys_header_size(key_count), GFP_KERNEL);
>>  	if (!keys_header)
>>  		return -ENOMEM;
>>@@ -412,7 +427,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>  		.top_down = false,
>>  		.random = true,
>>  	};
>>-	int r;
>>+	int r = 0;
>>  	if (key_count <= 0) {
>>@@ -421,14 +436,15 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>  	}
>>  	if (!is_dm_key_reused) {
>>-		image->dm_crypt_keys_addr = 0;
>>  		r = build_keys_header();
>>-		if (r) {
>>-			pr_err("Failed to build dm-crypt keys header, ret=%d\n", r);
>>-			return r;
>>-		}
>>+		if (r)
>>+			goto out;
>>  	}
>>+	/*
>>+	 * keys_header will be copied to reserver memory later and then be
>>+	 * cleaned up at the end of kexec_file_load syscall
>>+	 */
>>  	kbuf.buffer = keys_header;
>>  	kbuf.bufsz = get_keys_header_size(key_count);
>>@@ -438,18 +454,33 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>  	r = kexec_add_buffer(&kbuf);
>>  	if (r) {
>>  		pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
>>-		kvfree((void *)kbuf.buffer);
>>-		return r;
>>+		goto out;
>>  	}
>>+
>>  	image->dm_crypt_keys_addr = kbuf.mem;
>>  	image->dm_crypt_keys_sz = kbuf.bufsz;
>>  	kexec_dprintk(
>>  		"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
>>  		kbuf.bufsz, kbuf.memsz);
>>+out:
>>+	is_dm_key_reused = false;
>>  	return r;
>>  }
>>+void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>>+{
>>+	/*
>>+	 * For CPU/memory hot-plugging, the kdump image will be reloaded. Prevent
>>+	 * keys_header from being cleaned up during unloading when
>>+	 * is_dm_key_reused=true
>>+	 */
>>+	if (!is_dm_key_reused) {
>>+		kfree_sensitive(keys_header);
>>+		keys_header = NULL;
>
>Since crash_load_dm_crypt_keys() sets is_dm_key_reused = false, 
>keys_header will
>always be released here, right? Then why is the above free under an if 
>condition?

Thanks for raising the question! This is to prevent "kexec -u" from
cleaning up keys_headers because kexec_file_post_load_cleanup_dm_crypt
will also be called during "kexec -u". Without the if condition,
keys_headers will not be available during reloading.

>
>IIUC, for the case where CONFIG_CRASH_HOTPLUG is not enabled, this is 
>how key
>restore works:
>
>After loading the kdump kernel for the first time, the state of the 
>variables is:
>
>is_dm_key_reused = false
>keys_header = NULL
>
>For example, if 2 CPUs are hot-removed and kdump is reloaded twice:
>
>Then the sequence of operations needed to ensure the loaded keys can 
>be reused is:
>
>Udev rule triggered on the 1st CPU hotplug:
>echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>Restore the key header from the reserved area
>Reload the kdump service/kernel
>
>Udev rule triggered on the 2nd CPU hotplug:
>echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>Restore the key header from the reserved area
>Reload the kdump service/kernel
>
>What I don’t understand is the need to restore the key header from 
>crashkernel
>memory for every hotplug operation.

I think there is no easy way to tell if there is another hotplug and
considering hotplug is a rare event. So I guess it's OK to restore the
key headers for every hotplug operation.


-- 
Best regards,
Coiby

Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header

[Sourabh Jain]

On 08/05/26 18:03, Coiby Xu wrote:
> On Wed, May 06, 2026 at 05:58:31PM +0530, Sourabh Jain wrote:
>> Hello Coiby,
>
> Hi Sourabh,
>
> Thanks for reviewing the patch!
>
>>
>> On 02/05/26 05:13, Coiby Xu wrote:
>>> If kexec_add_buffer somehow fails, keys_header will be freed. Depending
>>> on /sys/kernel/config/crash_dm_crypt_key/reuse, it will lead to the
>>> following two problems if the kexec_file_load syscall is called again,
>>>   1. Double free of keys_header if reuse=false
>>>   2. UAF of keys_header if reuse=true
>>>
>>> To address these problems and also make it easier to reason about the
>>> code, keep two invariants,
>>>   1. keys_header will always be freed at the end of kexec_file_load
>>>      syscall except during kdump image unloading for CPU/memory
>>>      hot-plugging support
>>>   2. There will always be valid keys_header if reuse=true
>>>
>>> Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump 
>>> reserved memory")
>>> Fixes: 9ebfa8dcaea7 ("crash_dump: reuse saved dm crypt keys for 
>>> CPU/memory hot-plugging")
>>> Reported-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>>> ---
>>>  include/linux/kexec.h        |  6 ++++
>>>  kernel/crash_dump_dm_crypt.c | 65 ++++++++++++++++++++++++++----------
>>>  kernel/kexec_file.c          |  2 ++
>>>  3 files changed, 56 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>>> index 8a22bc9b8c6c..91256d7ff434 100644
>>> --- a/include/linux/kexec.h
>>> +++ b/include/linux/kexec.h
>>> @@ -552,6 +552,12 @@ void set_kexec_sig_enforced(void);
>>>  static inline void set_kexec_sig_enforced(void) {}
>>>  #endif
>>> +#ifdef CONFIG_CRASH_DM_CRYPT
>>> +void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image);
>>> +#else
>>> +static inline void kexec_file_post_load_cleanup_dm_crypt(struct 
>>> kimage *image) {}
>>> +#endif
>>> +
>>>  #endif /* !defined(__ASSEBMLY__) */
>>>  #endif /* LINUX_KEXEC_H */
>>> diff --git a/kernel/crash_dump_dm_crypt.c 
>>> b/kernel/crash_dump_dm_crypt.c
>>> index eac4f436a8d4..4d8a3331bbe7 100644
>>> --- a/kernel/crash_dump_dm_crypt.c
>>> +++ b/kernel/crash_dump_dm_crypt.c
>>> @@ -84,18 +84,25 @@ static int add_key_to_keyring(struct 
>>> dm_crypt_key *dm_key,
>>>      return r;
>>>  }
>>> -static void get_keys_from_kdump_reserved_memory(void)
>>> +static int get_keys_from_kdump_reserved_memory(void)
>>>  {
>>>      struct keys_header *keys_header_loaded;
>>> +    size_t keys_header_size;
>>> -    arch_kexec_unprotect_crashkres();
>>> +    keys_header_size = get_keys_header_size(key_count);
>>> +    keys_header = kzalloc(keys_header_size, GFP_KERNEL);
>>> +    if (!keys_header)
>>> +        return -ENOMEM;
>>> +    arch_kexec_unprotect_crashkres();
>>>      keys_header_loaded = kmap_local_page(pfn_to_page(
>>>          kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
>>
>> Accessing kexec_crash_image without holding the kexec lock could lead to
>> undefined behavior.
>>
>> I think this section of code should be called by holding kexec lock.
>>
>>
>>> -    memcpy(keys_header, keys_header_loaded, 
>>> get_keys_header_size(key_count));
>>> +    memcpy(keys_header, keys_header_loaded, keys_header_size);
>>>      kunmap_local(keys_header_loaded);
>>>      arch_kexec_protect_crashkres();
>>> +
>>> +    return 0;
>>>  }
>>>  static int restore_dm_crypt_keys_to_thread_keyring(void)
>>> @@ -283,17 +290,28 @@ static ssize_t config_keys_reuse_show(struct 
>>> config_item *item, char *page)
>>>  static ssize_t config_keys_reuse_store(struct config_item *item,
>>>                         const char *page, size_t count)
>>>  {
>>> +    bool val;
>>> +    int r;
>>> +
>>>      if (!kexec_crash_image || 
>>> !kexec_crash_image->dm_crypt_keys_addr) {
>>
>> The above check should be performed after acquiring the kexec lock.
>
> Good suggestion! I'll try to hold the kexec lock in next version.
>
>>
>>
>>
>>>          kexec_dprintk(
>>>              "dm-crypt keys haven't be saved to crash-reserved 
>>> memory\n");
>>>          return -EINVAL;
>>>      }
>>> -    if (kstrtobool(page, &is_dm_key_reused))
>>> +    if (kstrtobool(page, &val) || !val)
>>>          return -EINVAL;
>>
>> Why can’t we allow the user to set is_dm_key_reused = false and free the
>> key_header? That way, the next kdump kernel load will recreate the
>> For example, a user may add more keys and want the new keys to be 
>> included
>> in the kdump image from next kdump kernel load.
>
> Personally, I want to limit the reuse configfs API to the case of
> CPU/memory hotplug thus to make the code simpler. And for the case of a
> user adding more keys, I don't think there is a need for using the reuse
> API.


Yes, there is no need for it, but I think the user is forced to use
key_reuse because once it is set to true, it cannot be set back to false.

For example, the kdump kernel is loaded using the kexec_load system call
and reuse is set to true. The user may later add a couple of new keys and
want to include them in the kdump image.

I think the next kdump kernel load will reuse the key_headers even though
the user does not want it. Hence I see a problem here.

Well user can load kdump kernel a second time, and at that point the
keys will get updated. But do we really want this behavior?


>
>>
>>
>>> -    if (is_dm_key_reused)
>>> -        get_keys_from_kdump_reserved_memory();
>>> +    if (is_dm_key_reused) {
>>> +        pr_info("Already got dm-crypt keys, please continue with 
>>> kexec_file_load syscall\n");
>>> +    } else {
>>> +        r = get_keys_from_kdump_reserved_memory();
>>> +        if (r) {
>>> +            pr_warn("Failed to get dm-crypt keys from reserved 
>>> memory\n");
>>> +            return r;
>>> +        }
>>> +        is_dm_key_reused = true;
>>> +    }
>>>      return count;
>>>  }
>>> @@ -366,9 +384,6 @@ static int build_keys_header(void)
>>>      struct config_key *key;
>>>      int i, r;
>>> -    if (keys_header != NULL)
>>> -        kvfree(keys_header);
>>> -
>>>      keys_header = kzalloc(get_keys_header_size(key_count), 
>>> GFP_KERNEL);
>>>      if (!keys_header)
>>>          return -ENOMEM;
>>> @@ -412,7 +427,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>>          .top_down = false,
>>>          .random = true,
>>>      };
>>> -    int r;
>>> +    int r = 0;
>>>      if (key_count <= 0) {
>>> @@ -421,14 +436,15 @@ int crash_load_dm_crypt_keys(struct kimage 
>>> *image)
>>>      }
>>>      if (!is_dm_key_reused) {
>>> -        image->dm_crypt_keys_addr = 0;
>>>          r = build_keys_header();
>>> -        if (r) {
>>> -            pr_err("Failed to build dm-crypt keys header, 
>>> ret=%d\n", r);
>>> -            return r;
>>> -        }
>>> +        if (r)
>>> +            goto out;
>>>      }
>>> +    /*
>>> +     * keys_header will be copied to reserver memory later and then be
>>> +     * cleaned up at the end of kexec_file_load syscall
>>> +     */
>>>      kbuf.buffer = keys_header;
>>>      kbuf.bufsz = get_keys_header_size(key_count);
>>> @@ -438,18 +454,33 @@ int crash_load_dm_crypt_keys(struct kimage 
>>> *image)
>>>      r = kexec_add_buffer(&kbuf);
>>>      if (r) {
>>>          pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
>>> -        kvfree((void *)kbuf.buffer);
>>> -        return r;
>>> +        goto out;
>>>      }
>>> +
>>>      image->dm_crypt_keys_addr = kbuf.mem;
>>>      image->dm_crypt_keys_sz = kbuf.bufsz;
>>>      kexec_dprintk(
>>>          "Loaded dm crypt keys to kexec_buffer bufsz=0x%lx 
>>> memsz=0x%lx\n",
>>>          kbuf.bufsz, kbuf.memsz);
>>> +out:
>>> +    is_dm_key_reused = false;
>>>      return r;
>>>  }
>>> +void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>>> +{
>>> +    /*
>>> +     * For CPU/memory hot-plugging, the kdump image will be 
>>> reloaded. Prevent
>>> +     * keys_header from being cleaned up during unloading when
>>> +     * is_dm_key_reused=true
>>> +     */
>>> +    if (!is_dm_key_reused) {
>>> +        kfree_sensitive(keys_header);
>>> +        keys_header = NULL;
>>
>> Since crash_load_dm_crypt_keys() sets is_dm_key_reused = false, 
>> keys_header will
>> always be released here, right? Then why is the above free under an 
>> if condition?
>
> Thanks for raising the question! This is to prevent "kexec -u" from
> cleaning up keys_headers because kexec_file_post_load_cleanup_dm_crypt
> will also be called during "kexec -u". Without the if condition,
> keys_headers will not be available during reloading.

Agree. But do we really run kexec -u and then kexec -p to reload the kdump
kernel on hotplug events? I am under the impression that the udev rule
simply reloads the kdump kernel using kexec -p without explicitly running
kexec -u.

And if that is the case, I think we should clean key_headers on kexec -u.

- Sourabh Jain


>
>>
>> IIUC, for the case where CONFIG_CRASH_HOTPLUG is not enabled, this is 
>> how key
>> restore works:
>>
>> After loading the kdump kernel for the first time, the state of the 
>> variables is:
>>
>> is_dm_key_reused = false
>> keys_header = NULL
>>
>> For example, if 2 CPUs are hot-removed and kdump is reloaded twice:
>>
>> Then the sequence of operations needed to ensure the loaded keys can 
>> be reused is:
>>
>> Udev rule triggered on the 1st CPU hotplug:
>> echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>> Restore the key header from the reserved area
>> Reload the kdump service/kernel
>>
>> Udev rule triggered on the 2nd CPU hotplug:
>> echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>> Restore the key header from the reserved area
>> Reload the kdump service/kernel
>>
>> What I don’t understand is the need to restore the key header from 
>> crashkernel
>> memory for every hotplug operation.
>
> I think there is no easy way to tell if there is another hotplug and
> considering hotplug is a rare event. So I guess it's OK to restore the
> key headers for every hotplug operation. 

Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header

[Coiby Xu]

On Sat, May 09, 2026 at 01:36:59AM +0530, Sourabh Jain wrote:
>
>
>On 08/05/26 18:03, Coiby Xu wrote:
>>On Wed, May 06, 2026 at 05:58:31PM +0530, Sourabh Jain wrote:
>>>Hello Coiby,
>>
>>Hi Sourabh,
>>
>>Thanks for reviewing the patch!
>>
>>>
>>>On 02/05/26 05:13, Coiby Xu wrote:
>>>>If kexec_add_buffer somehow fails, keys_header will be freed. Depending
>>>>on /sys/kernel/config/crash_dm_crypt_key/reuse, it will lead to the
>>>>following two problems if the kexec_file_load syscall is called again,
>>>>  1. Double free of keys_header if reuse=false
>>>>  2. UAF of keys_header if reuse=true
>>>>
>>>>To address these problems and also make it easier to reason about the
>>>>code, keep two invariants,
>>>>  1. keys_header will always be freed at the end of kexec_file_load
>>>>     syscall except during kdump image unloading for CPU/memory
>>>>     hot-plugging support
>>>>  2. There will always be valid keys_header if reuse=true
>>>>
>>>>Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump 
>>>>reserved memory")
>>>>Fixes: 9ebfa8dcaea7 ("crash_dump: reuse saved dm crypt keys for 
>>>>CPU/memory hot-plugging")
>>>>Reported-by: Sourabh Jain <sourabhjain@linux.ibm.com>
[...]
>>
>>>
>>>
>>>
>>>>         kexec_dprintk(
>>>>             "dm-crypt keys haven't be saved to crash-reserved 
>>>>memory\n");
>>>>         return -EINVAL;
>>>>     }
>>>>-    if (kstrtobool(page, &is_dm_key_reused))
>>>>+    if (kstrtobool(page, &val) || !val)
>>>>         return -EINVAL;
>>>
>>>Why can’t we allow the user to set is_dm_key_reused = false and free the
>>>key_header? That way, the next kdump kernel load will recreate the
>>>For example, a user may add more keys and want the new keys to be 
>>>included
>>>in the kdump image from next kdump kernel load.
>>
>>Personally, I want to limit the reuse configfs API to the case of
>>CPU/memory hotplug thus to make the code simpler. And for the case of a
>>user adding more keys, I don't think there is a need for using the reuse
>>API.
>
>
>Yes, there is no need for it, but I think the user is forced to use
>key_reuse because once it is set to true, it cannot be set back to false.
>
>For example, the kdump kernel is loaded using the kexec_load system call
>and reuse is set to true. The user may later add a couple of new keys and
>want to include them in the kdump image.
>
>I think the next kdump kernel load will reuse the key_headers even though
>the user does not want it. Hence I see a problem here.
>
>Well user can load kdump kernel a second time, and at that point the
>keys will get updated. But do we really want this behavior?

key_reuse won't be set to true automatically unless an user explicitly
does so. Thus I don't think it's forcing users to use it. And by design
key_reuse is set only for the hotplug case.

For the cause of loading kdump kernel again, I don't see the need to set
key_reuse.

I also had the urge to allow writing false to the reuse API and even
implemented it. But later I deleted the code to avoid over-engineering
because I'm not sure how it is supposed to be used. Besides this reuse
API already makes it much harder to reason about the code. Shall all
architectures implement CONFIG_CRASH_HOTPLUG, I'll delete it immediately
so crash_dump_dm_crypt.c can be much easier to maintain.

>
>
>>
>>>
>>>
>>>>-    if (is_dm_key_reused)
>>>>-        get_keys_from_kdump_reserved_memory();
>>>>+    if (is_dm_key_reused) {
>>>>+        pr_info("Already got dm-crypt keys, please continue 
>>>>with kexec_file_load syscall\n");
>>>>+    } else {
>>>>+        r = get_keys_from_kdump_reserved_memory();
>>>>+        if (r) {
>>>>+            pr_warn("Failed to get dm-crypt keys from reserved 
>>>>memory\n");
>>>>+            return r;
>>>>+        }
>>>>+        is_dm_key_reused = true;
>>>>+    }
>>>>     return count;
>>>> }
>>>>@@ -366,9 +384,6 @@ static int build_keys_header(void)
>>>>     struct config_key *key;
>>>>     int i, r;
>>>>-    if (keys_header != NULL)
>>>>-        kvfree(keys_header);
>>>>-
>>>>     keys_header = kzalloc(get_keys_header_size(key_count), 
>>>>GFP_KERNEL);
>>>>     if (!keys_header)
>>>>         return -ENOMEM;
>>>>@@ -412,7 +427,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>>>         .top_down = false,
>>>>         .random = true,
>>>>     };
>>>>-    int r;
>>>>+    int r = 0;
>>>>     if (key_count <= 0) {
>>>>@@ -421,14 +436,15 @@ int crash_load_dm_crypt_keys(struct kimage 
>>>>*image)
>>>>     }
>>>>     if (!is_dm_key_reused) {
>>>>-        image->dm_crypt_keys_addr = 0;
>>>>         r = build_keys_header();
>>>>-        if (r) {
>>>>-            pr_err("Failed to build dm-crypt keys header, 
>>>>ret=%d\n", r);
>>>>-            return r;
>>>>-        }
>>>>+        if (r)
>>>>+            goto out;
>>>>     }
>>>>+    /*
>>>>+     * keys_header will be copied to reserver memory later and then be
>>>>+     * cleaned up at the end of kexec_file_load syscall
>>>>+     */
>>>>     kbuf.buffer = keys_header;
>>>>     kbuf.bufsz = get_keys_header_size(key_count);
>>>>@@ -438,18 +454,33 @@ int crash_load_dm_crypt_keys(struct kimage 
>>>>*image)
>>>>     r = kexec_add_buffer(&kbuf);
>>>>     if (r) {
>>>>         pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
>>>>-        kvfree((void *)kbuf.buffer);
>>>>-        return r;
>>>>+        goto out;
>>>>     }
>>>>+
>>>>     image->dm_crypt_keys_addr = kbuf.mem;
>>>>     image->dm_crypt_keys_sz = kbuf.bufsz;
>>>>     kexec_dprintk(
>>>>         "Loaded dm crypt keys to kexec_buffer bufsz=0x%lx 
>>>>memsz=0x%lx\n",
>>>>         kbuf.bufsz, kbuf.memsz);
>>>>+out:
>>>>+    is_dm_key_reused = false;
>>>>     return r;
>>>> }
>>>>+void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>>>>+{
>>>>+    /*
>>>>+     * For CPU/memory hot-plugging, the kdump image will be 
>>>>reloaded. Prevent
>>>>+     * keys_header from being cleaned up during unloading when
>>>>+     * is_dm_key_reused=true
>>>>+     */
>>>>+    if (!is_dm_key_reused) {
>>>>+        kfree_sensitive(keys_header);
>>>>+        keys_header = NULL;
>>>
>>>Since crash_load_dm_crypt_keys() sets is_dm_key_reused = false, 
>>>keys_header will
>>>always be released here, right? Then why is the above free under 
>>>an if condition?
>>
>>Thanks for raising the question! This is to prevent "kexec -u" from
>>cleaning up keys_headers because kexec_file_post_load_cleanup_dm_crypt
>>will also be called during "kexec -u". Without the if condition,
>>keys_headers will not be available during reloading.
>
>Agree. But do we really run kexec -u and then kexec -p to reload the kdump
>kernel on hotplug events? I am under the impression that the udev rule
>simply reloads the kdump kernel using kexec -p without explicitly running
>kexec -u.

Yes, "kdumpctl reload" will be called during hotplug events 
https://github.com/rhkdump/kdump-utils/blob/main/kdump-udev-throttler#L51
So there is explicitly unloading first and then reloading.

>
>And if that is the case, I think we should clean key_headers on kexec -u.

Most of the cases is_dm_key_reused is false thus keys_header will be
cleaned on kexec -u.

>
>- Sourabh Jain
>
>
>>
>>>
>>>IIUC, for the case where CONFIG_CRASH_HOTPLUG is not enabled, this 
>>>is how key
>>>restore works:
>>>
>>>After loading the kdump kernel for the first time, the state of 
>>>the variables is:
>>>
>>>is_dm_key_reused = false
>>>keys_header = NULL
>>>
>>>For example, if 2 CPUs are hot-removed and kdump is reloaded twice:
>>>
>>>Then the sequence of operations needed to ensure the loaded keys 
>>>can be reused is:
>>>
>>>Udev rule triggered on the 1st CPU hotplug:
>>>echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>>>Restore the key header from the reserved area
>>>Reload the kdump service/kernel
>>>
>>>Udev rule triggered on the 2nd CPU hotplug:
>>>echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>>>Restore the key header from the reserved area
>>>Reload the kdump service/kernel
>>>
>>>What I don’t understand is the need to restore the key header from 
>>>crashkernel
>>>memory for every hotplug operation.
>>
>>I think there is no easy way to tell if there is another hotplug and
>>considering hotplug is a rare event. So I guess it's OK to restore the
>>key headers for every hotplug operation.
>
>

-- 
Best regards,
Coiby

Re: [PATCH v2 3/9] crash_dump: Disallow writing to dm-crypt configfs during kexec_file_load syscall

[Sourabh Jain]

On 02/05/26 05:13, Coiby Xu wrote:
> If writing to the configfs group happens concurrently during
> kexec_file_load syscall, it may lead to the following issues,
>    - buffer overflow if dm-crypt keys are added after allocation
>    - stale total_keys if dm-crypt keys are removed during iteration
>    - keys_header will not be freed if config/crash_dm_crypt_key/reuse is
>      set true
>
> So hold config_keys_subsys.su_mutex for the entire sequence during the
> kexec_file_load syscall to ensure a consistent snapshot.


Yes, this will solve many synchronization problems when a  user tries to
perform any operation under our configfs_subsystem while the kernel is
executing the kexec_file_load system call.

Now, regarding the third point about freeing key_header: this will work
only if configfs takes the su_mutex lock before invoking the store callback.
I am not sure whether it actually does.

However, based on my previous comment on (2/9), if we keep 
config_keys_reuse_store()
under the kexec lock, this will be taken care of. This is because the entire
kexec_file_load system call already runs under the kexec lock.


>
> Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump reserved memory")
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   kernel/crash_dump_dm_crypt.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 4d8a3331bbe7..6377ee86ec50 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -429,6 +429,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	};
>   	int r = 0;
>   
> +	mutex_lock(&config_keys_subsys.su_mutex);
>   
>   	if (key_count <= 0) {
>   		kexec_dprintk("No dm-crypt keys\n");
> @@ -479,6 +480,9 @@ void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>   		kfree_sensitive(keys_header);
>   		keys_header = NULL;
>   	}
> +
> +	if (mutex_is_locked(&config_keys_subsys.su_mutex))
> +		mutex_unlock(&config_keys_subsys.su_mutex);
How about release the lock in crash_load_dm_crypt_keys() itself? Given 
that config_keys_reuse_store() is placed under kexec lock?


- Sourabh Jain

>   }
>   
>   static int __init configfs_dmcrypt_keys_init(void)

Re: [PATCH v2 3/9] crash_dump: Disallow writing to dm-crypt configfs during kexec_file_load syscall

[Coiby Xu]

On Wed, May 06, 2026 at 07:26:16PM +0530, Sourabh Jain wrote:
>
>
>On 02/05/26 05:13, Coiby Xu wrote:
>>If writing to the configfs group happens concurrently during
>>kexec_file_load syscall, it may lead to the following issues,
>>   - buffer overflow if dm-crypt keys are added after allocation
>>   - stale total_keys if dm-crypt keys are removed during iteration
>>   - keys_header will not be freed if config/crash_dm_crypt_key/reuse is
>>     set true
>>
>>So hold config_keys_subsys.su_mutex for the entire sequence during the
>>kexec_file_load syscall to ensure a consistent snapshot.
>
>
>Yes, this will solve many synchronization problems when a  user tries to
>perform any operation under our configfs_subsystem while the kernel is
>executing the kexec_file_load system call.
>
>Now, regarding the third point about freeing key_header: this will work
>only if configfs takes the su_mutex lock before invoking the store callback.
>I am not sure whether it actually does.

I can confirm configfs will automatically acquire the su_mutex lock when
creating configfs item. But I forgot to try if writing to an item will
also acquire the mutux lock. I'll do an experiment and share the result
later.

>
>However, based on my previous comment on (2/9), if we keep 
>config_keys_reuse_store()
>under the kexec lock, this will be taken care of. This is because the entire
>kexec_file_load system call already runs under the kexec lock.
>
>
>>
>>Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump reserved memory")
>>Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>>---
>>  kernel/crash_dump_dm_crypt.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>>diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>>index 4d8a3331bbe7..6377ee86ec50 100644
>>--- a/kernel/crash_dump_dm_crypt.c
>>+++ b/kernel/crash_dump_dm_crypt.c
>>@@ -429,6 +429,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>  	};
>>  	int r = 0;
>>+	mutex_lock(&config_keys_subsys.su_mutex);
>>  	if (key_count <= 0) {
>>  		kexec_dprintk("No dm-crypt keys\n");
>>@@ -479,6 +480,9 @@ void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>>  		kfree_sensitive(keys_header);
>>  		keys_header = NULL;
>>  	}
>>+
>>+	if (mutex_is_locked(&config_keys_subsys.su_mutex))
>>+		mutex_unlock(&config_keys_subsys.su_mutex);
>How about release the lock in crash_load_dm_crypt_keys() itself? Given 
>that config_keys_reuse_store() is placed under kexec lock?

Thanks for proposing another solution! I'll give a try and make a
comparison.

-- 
Best regards,
Coiby

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

[Sourabh Jain]

On 02/05/26 05:13, 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 6377ee86ec50..a3e460714d23 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;
> +	}

Yes it is good to do a sanity check before using it.

- Sourabh Jain

> +
> +	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)
> @@ -431,12 +441,12 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   
>   	mutex_lock(&config_keys_subsys.su_mutex);
>   
> -	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;
> +		}
> +
>   		r = build_keys_header();
>   		if (r)
>   			goto out;
> @@ -447,7 +457,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;

Re: [PATCH v2 7/9] crash_dump: Improve readability of config_keys_restore_store

[Sourabh Jain]

On 02/05/26 05:13, Coiby Xu wrote:
> config_keys_restore_store currently doesn't validate the user input
> before restoring dm-crypt keys. Although it's not necessary for the case
> of vmcore dumping, it's better to do it for the sake of consistency and
> code readability. Also check the return code of
> restore_dm_crypt_keys_to_thread_keyring.
>
> Fixes: 62f17d9df692 ("crash_dump: retrieve dm crypt keys in kdump kernel")
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   kernel/crash_dump_dm_crypt.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 545d712428d3..36e51807d94f 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -358,12 +358,16 @@ static ssize_t config_keys_restore_show(struct config_item *item, char *page)
>   static ssize_t config_keys_restore_store(struct config_item *item,
>   					  const char *page, size_t count)
>   {
> -	if (!restore)
> -		restore_dm_crypt_keys_to_thread_keyring();
> +	bool val;
>   
> -	if (kstrtobool(page, &restore))
> +	if (kstrtobool(page, &val))
>   		return -EINVAL;
>   
> +	if (val && !restore) {
> +		if (!restore_dm_crypt_keys_to_thread_keyring())
> +			restore = true;
> +	}

Nit:
I think it would be good to warn the user if they try to
restore it when it has already been restored.


- Sourabh Jain
> +
>   	return count;
>   }
>   

Re: [PATCH v2 8/9] crash_dump: Disallow configfs/crash_dm_crypt_key/reuse if CONFIG_CRASH_HOTPLUG enabled

[Sourabh Jain]

On 02/05/26 05:13, Coiby Xu wrote:
> If CONFIG_CRASH_HOTPLUG is enabled, dm-crypt keys saved to reserved
> memory will be took care of automatically. Thus it doesn't make sense
> to use configfs/crash_dm_crypt_key/reuse. Reserving
> image->dm_crypt_keys_addr is also unnecessary. Currently x86_64 and
> ppc64le have implemented CONFIG_CRASH_HOTPLUG feature.
>
> Also update the doc accordingly. Note two doc issues are fixed as well.
>
> Fixes: 9ebfa8dcaea7 ("crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging")
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   Documentation/admin-guide/kdump/kdump.rst |  9 ++++++---
>   kernel/crash_dump_dm_crypt.c              | 14 +++++++++++---
>   2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/admin-guide/kdump/kdump.rst b/Documentation/admin-guide/kdump/kdump.rst
> index 7587caadbae1..73f2e9500c60 100644
> --- a/Documentation/admin-guide/kdump/kdump.rst
> +++ b/Documentation/admin-guide/kdump/kdump.rst
> @@ -577,9 +577,10 @@ with /sys/kernel/config/crash_dm_crypt_keys for setup,
>   
>   1. Tell the first kernel what logon keys are needed to unlock the disk volumes,
>       # Add key #1
> -    mkdir /sys/kernel/config/crash_dm_crypt_keys/7d26b7b4-e342-4d2d-b660-7426b0996720
> +    VOL1_UUID=7d26b7b4-e342-4d2d-b660-7426b0996720
> +    mkdir /sys/kernel/config/crash_dm_crypt_keys/$VOL1_UUID
>       # Add key #1's description
> -    echo cryptsetup:7d26b7b4-e342-4d2d-b660-7426b0996720 > /sys/kernel/config/crash_dm_crypt_keys/description
> +    echo cryptsetup:$VOL1_UUID > /sys/kernel/config/crash_dm_crypt_keys/$VOL1_UUID/description
>   
>       # how many keys do we have now?
>       cat /sys/kernel/config/crash_dm_crypt_keys/count
> @@ -593,7 +594,9 @@ with /sys/kernel/config/crash_dm_crypt_keys for setup,
>   
>       # To support CPU/memory hot-plugging, reuse keys already saved to reserved
>       # memory
> -    echo true > /sys/kernel/config/crash_dm_crypt_key/reuse
> +    # Note if CONFIG_CRASH_HOTPLUG is enabled, this API is totally unnecessary
> +    # thus will be disabled.
> +    echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
>   
>   2. Load the dump-capture kernel
>   
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 36e51807d94f..7a7cae17f578 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -304,6 +304,11 @@ static ssize_t config_keys_reuse_store(struct config_item *item,
>   	bool val;
>   	int r;
>   
> +	if (IS_ENABLED(CONFIG_CRASH_HOTPLUG)) {
> +		pr_info("CONFIG_CRASH_HOTPLUG already enabled");
> +		return -EINVAL;
> +	}
> +

Deciding this solely at compile time can create issues. For example, the 
kernel
may be built with CONFIG_CRASH_HOTPLUG, but if kexec tool loads the kdump
kernel using the kexec_load system call without hotplug support, it can
cause problems. It is rare but possible.

How about this:

#ifdef CONFIG_CRASH_HOTPLUG
     if (kexec_crash_image->hotplug_support) {
pr_info("crash image is loaded with hotplug support\n");return -EINVAL;
     }
#endif

This code should be placed after validating kexec_crash_image.


- Sourabh Jain

>   	if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {
>   		pr_info("dm-crypt keys haven't be saved to crash-reserved memory\n");
>   		return -EINVAL;
> @@ -486,15 +491,18 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>   {
>   	/*
> -	 * For CPU/memory hot-plugging, the kdump image will be reloaded. Prevent
> -	 * keys_header from being cleaned up during unloading when
> -	 * is_dm_key_reused=true
> +	 * For CPU/memory hot-plugging without CONFIG_CRASH_HOTPLUG, the whole kdump
> +	 * image will be reloaded. Prevent keys_header from being cleaned up during
> +	 * unloading when is_dm_key_reused=true
>   	 */
>   	if (!is_dm_key_reused) {
>   		kfree_sensitive(keys_header);
>   		keys_header = NULL;
>   	}
>   
> +	if (IS_ENABLED(CONFIG_CRASH_HOTPLUG))
> +		image->dm_crypt_keys_addr = 0;
> +
>   	if (mutex_is_locked(&config_keys_subsys.su_mutex))
>   		mutex_unlock(&config_keys_subsys.su_mutex);
>   }