[PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header
Sourabh Jain
sourabhjain at linux.ibm.com
Wed May 6 05:28:31 PDT 2026
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 at linux.ibm.com>
> Signed-off-by: Coiby Xu <coiby.xu at 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;
> }
>
More information about the kexec
mailing list