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

[Sourabh Jain]

Hello Coiby,

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@xxxxxxxxxxxxxxx
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Reported-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
> Signed-off-by: Coiby Xu <coxu@xxxxxxxxxx>
> ---
>   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


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.

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.

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?

5. Related to releasing the keyring_ref reference count, but
   I did not fully understand this concern.

6. restore_dm_crypt_keys_to_thread_keyring() does not release
   previously allocated keys_header, leading to a memory leak.

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.


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 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@xxxxxxxxxx/

- Sourabh Jain