Re: [PATCH v4 1/9] crash_dump: Fix potential double free and UAF of keys_header
From: Jinjie Ruan
Date: Mon Aug 31 2026 - 21:36:35 EST
在 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@xxxxxxxxxxxxx>
>>> Signed-off-by: Coiby Xu <coiby.xu@xxxxxxxxx>
>>> ---
>>> 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,
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,
-> 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.
Best regards,
Jinjie
> [...]
>