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

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

[Coiby Xu]

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
-- 
2.53.0

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


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@redhat.com/

- Sourabh Jain

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

[Coiby Xu]

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

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

[Sourabh Jain]

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