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

[Coiby Xu <coiby.xu@gmail.com>]

On Tue, Sep 01, 2026 at 09:36:10AM +0800, Jinjie Ruan wrote:
>
>
>在 2026/8/31 21:44, Coiby Xu 写道:
>> On Mon, Aug 31, 2026 at 03:39:35PM +0800, Jinjie Ruan wrote:
>>>
>>>
>>> 在 2026/8/28 16:48, Coiby Xu 写道:
>>>> 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 | 66 ++++++++++++++++++++++++++----------
>>>>  kernel/kexec_file.c          |  2 ++
>>>>  3 files changed, 56 insertions(+), 18 deletions(-)
>>>>
>
>[...]
>
>>>> @@ -369,9 +387,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;
>>>> @@ -415,8 +430,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>>>          .top_down = false,
>>>>          .random = true,
>>>>      };
>>>> -    int r;
>>>> -
>>>> +    int r = 0;
>>>>
>>>>      if (key_count <= 0) {
>>>>          kexec_dprintk("No dm-crypt keys\n");
>>>> @@ -424,14 +438,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);
>>>>
>>>> @@ -441,18 +456,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;
>>>
>>> I think there's still a bug for arm64 here.
>>>
>>> When is_dm_key_reused set to true, the outer while loop prematurely
>>> clears the is_dm_key_reused flag at the end of
>>> crash_load_dm_crypt_keys() in the very first iteration.
>>>
>>> Consequently, subsequent re-entrant calls to crash_load_dm_crypt_keys()
>>> falsely perceive the keys as not reused, forcing a redundant execution
>>> of build_keys_header() which overwrites the global keys_header pointer;
>>>
>>> This induces a silent kernel memory leak.
>>
>> Thanks for reviewing this patch and raising the concern! Unless I miss
>> something, I don't think the global keys_header will be overwritten
>> because kexec_file_post_load_cleanup_dm_crypt will called to free the
>> memory during the end of kexec_file_load syscall.
>
>Hi Coiby,

Hi Jinjie,

>
>Not really on arm64, as below:
>
>kexec_file_load syscall
>-> kimage_file_alloc_init()
>   -> kimage_file_prepare_segments()
>      -> kexec_image_load_default()
>         -> arm64 image_load()
>            -> load_other_segments() while retry loop
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>               -> crash_load_dm_crypt_keys()  -> May be called
>repeatedly!!! Memory leak bug here as I described above,

Thanks for providing the stack trace to help me understand the problem!
Now I see what I missed. I also notice your 
"[PATCH v3 04/17] arm64: kexec_file: Fix elf_headers memory leak in retry loop" 
is addressing the same problem for the elf_headers. I'll have a careful
look at related patches and re-think the solution.

>
>      -> kimage_file_post_load_cleanup()
>
>>
>>>
>>> 92 >-------/*
>>> 93 >------- * The location of the kernel segment may make it impossible
>>> to satisfy
>>> 94 >------- * the other segment requirements, so we try repeatedly to
>>> find a
>>> 95 >------- * location that will work.
>>> 96 >------- */
>>> 97 >-------while ((ret = kexec_add_buffer(&kbuf)) == 0) {
>>> 98 >------->-------/* Try to load additional data */
>>> 99 >------->-------kernel_segment = &image-
>>> >segment[kernel_segment_number];
>>> 100 >------->-------ret = load_other_segments(image, kernel_segment->mem,
>>> 101 >------->------->------->------->-------  kernel_segment->memsz,
>>> initrd,
>>> 102 >------->------->------->------->-------  initrd_len, cmdline);
>>> 103 >------->-------if (!ret)
>>> 104 >------->------->-------break;
>>> 105
>>> 106 >------->-------/*
>>> 107 >------->------- * We couldn't find space for the other segments;
>>> erase the
>>> 108 >------->------- * kernel segment and try the next available hole.
>>> 109 >------->------- */
>>> 110 >------->-------image->nr_segments -= 1;
>>> 111 >------->-------kbuf.buf_min = kernel_segment->mem +
>>> kernel_segment->memsz;
>>> 112 >------->-------kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>> 113 >-------}
>>
>> Btw, I assume the above text is not related to the discussion, right?
>
>The code closely related to my description above — the arm64
>crash_load_dm_crypt_keys() — is exactly called within the
>load_other_segments() in the while loop mentioned earlier. Therefore,
>crash_load_dm_crypt_keys() may be called repeatedly before
>kexec_file_post_load_cleanup_dm_crypt().
>
>We can refer to the arm64 commit 108aa503657e ("arm64: kexec_file: try
>more regions if loading segments fails") , which introduced the retry loop.

Thanks for the explanation and also pointing me to the reference commit!

>
>Best regards,
>Jinjie
>
>> [...]
>>
>

-- 
Best regards,
Coiby