Re: [PATCH] crash_dump: Fix potential double free and UAF of keys_header

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

On 07/04/26 06:14, Coiby Xu wrote:
> On Fri, Apr 03, 2026 at 07:48:29PM +0530, Sourabh Jain wrote:
>> Hello Coiby,
>
> Hi Sourabh,
>
>>
>> On 03/04/26 15:31, Coiby Xu wrote:
>>> If kexec_add_buffer fails, keys_header will be freed. And 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
>>>
>>> Address these problems by setting keys_header to NULL after freeing
>>> kbuf.buffer and re-building keys_header when necessary respectively.
>>>
>>> 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")
>>> Cc: stable@vger.kernel.org
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Reported-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>>> Signed-off-by: Coiby Xu <coxu@redhat.com>
>>> ---
>>>  kernel/crash_dump_dm_crypt.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/crash_dump_dm_crypt.c 
>>> b/kernel/crash_dump_dm_crypt.c
>>> index a20d4097744a..92eebef27156 100644
>>> --- a/kernel/crash_dump_dm_crypt.c
>>> +++ b/kernel/crash_dump_dm_crypt.c
>>> @@ -417,7 +417,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>>          return -ENOENT;
>>>      }
>>> -    if (!is_dm_key_reused) {
>>> +    if (!is_dm_key_reused || !keys_header) {
>>>          image->dm_crypt_keys_addr = 0;
>>>          r = build_keys_header();
>>>          if (r)
>>> @@ -433,6 +433,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>>>      r = kexec_add_buffer(&kbuf);
>>>      if (r) {
>>>          kvfree((void *)kbuf.buffer);
>>> +        keys_header = NULL;
>>>          return r;
>>>      }
>>>      image->dm_crypt_keys_addr = kbuf.mem;
>>>
>>> base-commit: d8a9a4b11a137909e306e50346148fc5c3b63f9d
>>
>> Sashiko raised seven concerns on this patch. Most of them are
>> not directly related to the changes introduced here, but I
>> think they can be addressed along with this fix.
>>
>> https://sashiko.dev/#/patchset/20260403100126.1468200-1-coxu%40redhat.com 
>>
>
> Thanks for pointing me to the Sashiko's code review and also sharing
> your meticulous analysis!
>
>>
>>
>> 1. build_keys_header() does not release key_header memory on
>>    error. This can cause incorrect keys to be loaded for the
>>    kdump kernel in subsequent system calls.
>>
>> Can be addressed by releasing keys_header on error path.
>
> I'll address this issue! Thanks for the suggestion!
>
>>
>> 2–3. get_keys_header_size() uses key_count to find the size of
>> key_header buffer, which can lead to out-of-bounds access
>> at two places.
>>   a. Around kexec_add_buffer()
>>   b. In build_keys_header()
>>
>> I think there is one more place where this applies is:
>>   c. In get_keys_from_kdump_reserved_memory() at memcpy
>>
>> I agree with solution provided by Sashiko of using 
>> keys_header->total_keys
>> instead.
>
> Thanks for showing me where out-of-bounds accesses can happen! I'll do
> some testing to see if using keys_header->total_keys is sufficient.
>
>>
>> 4. get_keys_from_kdump_reserved_memory() may run into issues
>>    if kexec_crash_image->dm_crypt_keys_addr is larger than a
>>    page size during memcpy. Because kmap_local_page only maps
>>    one page.
>>
>> How about moving this in a loop and do map and copy page by page?
>
> Yeah, looping over the pages should be a robust solution.
>
>>
>> 5. Related to releasing the keyring_ref reference count, but
>>    I did not fully understand this concern.
>
> My latest test already covers the case where there are two keys to
> iterate over. I'll dig more into keyring_ref to see if Sashiko's
> concerns is valid.
>
>>
>> 6. restore_dm_crypt_keys_to_thread_keyring() does not release
>>    previously allocated keys_header, leading to a memory leak.
>
> Thanks for raising the concern! Although we can assume the system will
> reboot soon after vmcore dumping is finished, it's better to free
> keys_header.
>
>>
>> As per kdump.rst, restore was introduced to handle CPU and
>> memory hotplug cases. Is it needed when there is no in-kernel
>> update to the kdump image on CPU or memory hotplug events?
>>
>> But in that case, we rely on a udev rule to reload the kdump image
>> again.
>>
>> I am confused about when exactly we need to restore.
>
> To clarify, reuse other than restore is needed for non in-kernel update
> when handing CPU/memory hotplugging. Yes, a udev rule is also needed in
> this case.

Below commit explains how the reuse is utilized:

commit 9ebfa8dcaea77a8ef02d0f9478717a138b0ad828
Author: Coiby Xu <coxu@redhat.com>
Date:   Fri May 2 09:12:38 2025 +0800

     crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging

It got it now. This is helpful when kdump needs to be reloaded due to
CPU/memory hotplug events using the kexec_file_load system call,
but only when CONFIG_CRASH_HOTPLUG is not enabled.

IIUC this feature is not support on crash image loaded using kexec_load 
syscall, right?


- Sourabh Jain