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

[Coiby Xu <coxu@redhat.com>]

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.

For restore, it's to restore dm-crypt keys in kdump kernel. I'll see if
I can update the documentation to improve clarity.

>
>
>7. Possible memory leak and data races due to concurrent kexec loads.
>
>I think we can ignore this because both kexec system calls are protected
>by the same lock.

I agree, this concern can be dismissed.

>
>I also noticed that kdump.rst still says CONFIG_CRASH_DM_CRYPT is
>only supported on x86_64 for now. With the patch series below,
>this needs to change, right?
>https://lore.kernel.org/all/20260225060347.718905-1-coxu@redhat.com/

Yes, the documentation will need to updated. Thanks for the reminder!

>
>- Sourabh Jain
>
>
>
>

-- 
Best regards,
Coiby