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

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

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.