Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header

On Tue, May 12, 2026 at 11:12:16AM +0530, Sourabh Jain wrote:
>
>
>On 10/05/26 05:44, Coiby Xu wrote:
>>On Sat, May 09, 2026 at 01:36:59AM +0530, Sourabh Jain wrote:
>>>
>>>
>>>On 08/05/26 18:03, Coiby Xu wrote:
>>>>On Wed, May 06, 2026 at 05:58:31PM +0530, Sourabh Jain wrote:
>>>>>Hello Coiby,
>>>>
>>>>Hi Sourabh,
>>>>
>>>>Thanks for reviewing the patch!
>>>>
>>>>>
>>>>>On 02/05/26 05:13, Coiby Xu wrote:
>>>>>>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 <[email protected]>
>>[...]
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>         kexec_dprintk(
>>>>>>             "dm-crypt keys haven't be saved to 
>>>>>>crash-reserved memory\n");
>>>>>>         return -EINVAL;
>>>>>>     }
>>>>>>-    if (kstrtobool(page, &is_dm_key_reused))
>>>>>>+    if (kstrtobool(page, &val) || !val)
>>>>>>         return -EINVAL;
>>>>>
>>>>>Why can’t we allow the user to set is_dm_key_reused = false 
>>>>>and free the
>>>>>key_header? That way, the next kdump kernel load will recreate the
>>>>>For example, a user may add more keys and want the new keys to 
>>>>>be included
>>>>>in the kdump image from next kdump kernel load.
>>>>
>>>>Personally, I want to limit the reuse configfs API to the case of
>>>>CPU/memory hotplug thus to make the code simpler. And for the case of a
>>>>user adding more keys, I don't think there is a need for using 
>>>>the reuse
>>>>API.
>>>
>>>
>>>Yes, there is no need for it, but I think the user is forced to use
>>>key_reuse because once it is set to true, it cannot be set back to 
>>>false.
>>>
>>>For example, the kdump kernel is loaded using the kexec_load system call
>>>and reuse is set to true. The user may later add a couple of new 
>>>keys and
>>>want to include them in the kdump image.
>>>
>>>I think the next kdump kernel load will reuse the key_headers even 
>>>though
>>>the user does not want it. Hence I see a problem here.
>>>
>>>Well user can load kdump kernel a second time, and at that point the
>>>keys will get updated. But do we really want this behavior?
>>
>>key_reuse won't be set to true automatically unless an user explicitly
>>does so. Thus I don't think it's forcing users to use it. And by design
>>key_reuse is set only for the hotplug case.
>
>Oh, I got it now. So, on kdump kernel load, key reuse will always be
>set to false.
>
>And when the user wants to reuse the key, they can just set the key reuse to
>true and reload the kdump kernel. After the reload, key reuse will be set to
>false again by the kernel itself.
>
>Thanks for clearing up my doubts.

My pleasure! Good to know we are on the same page now:)

>
[...]
>>>>>>+    if (!is_dm_key_reused) {
>>>>>>+        kfree_sensitive(keys_header);
>>>>>>+        keys_header = NULL;
>>>>>
>>>>>Since crash_load_dm_crypt_keys() sets is_dm_key_reused = 
>>>>>false, keys_header will
>>>>>always be released here, right? Then why is the above free 
>>>>>under an if condition?
>>>>
>>>>Thanks for raising the question! This is to prevent "kexec -u" from
>>>>cleaning up keys_headers because kexec_file_post_load_cleanup_dm_crypt
>>>>will also be called during "kexec -u". Without the if condition,
>>>>keys_headers will not be available during reloading.
>>>
>>>Agree. But do we really run kexec -u and then kexec -p to reload 
>>>the kdump
>>>kernel on hotplug events? I am under the impression that the udev rule
>>>simply reloads the kdump kernel using kexec -p without explicitly 
>>>running
>>>kexec -u.
>>
>>Yes, "kdumpctl reload" will be called during hotplug events https://github.com/rhkdump/kdump-utils/blob/main/kdump-udev-throttler#L51
>>So there is explicitly unloading first and then reloading.
>
>Oh ok, then it makes sense.
>
>BTW, I got the impression about not using kexec -u on kdump service reload
>from the repo below:
>
>https://github.com/openSUSE/kdump/blob/master/70-kdump.rules.in [Udev Rule]
>https://github.com/openSUSE/kdump/blob/00979dc621ba35285b85bf55917a5b0dfd273114/init/load-once.sh#L22 
>[Wrapper to load once for multiple hotplug]
>https://github.com/openSUSE/kdump/blob/00979dc621ba35285b85bf55917a5b0dfd273114/init/load.sh#L312 
>[load kdump script]

Thanks for sharing how openSUSE support hotplug! I believe current API
shall also work for openSUSE if they plan to support LUKS-encrypted dump
target.

-- 
Best regards,
Coiby