Re: [PATCH v2 2/9] crash_dump: Fix potential double free and UAF of keys_header
From: Sourabh Jain
Date: Tue May 12 2026 - 01:54:39 EST
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 <sourabhjain@xxxxxxxxxxxxx>
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.
For the cause of loading kdump kernel again, I don't see the need to set
key_reuse.
I also had the urge to allow writing false to the reuse API and even
implemented it. But later I deleted the code to avoid over-engineering
because I'm not sure how it is supposed to be used. Besides this reuse
API already makes it much harder to reason about the code. Shall all
architectures implement CONFIG_CRASH_HOTPLUG, I'll delete it immediately
so crash_dump_dm_crypt.c can be much easier to maintain.
- if (is_dm_key_reused)
- get_keys_from_kdump_reserved_memory();
+ if (is_dm_key_reused) {
+ pr_info("Already got dm-crypt keys, please continue with kexec_file_load syscall\n");
+ } else {
+ r = get_keys_from_kdump_reserved_memory();
+ if (r) {
+ pr_warn("Failed to get dm-crypt keys from reserved memory\n");
+ return r;
+ }
+ is_dm_key_reused = true;
+ }
return count;
}
@@ -366,9 +384,6 @@ static int build_keys_header(void)
struct config_key *key;
int i, r;
- if (keys_header != NULL)
- kvfree(keys_header);
-
keys_header = kzalloc(get_keys_header_size(key_count), GFP_KERNEL);
if (!keys_header)
return -ENOMEM;
@@ -412,7 +427,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
.top_down = false,
.random = true,
};
- int r;
+ int r = 0;
if (key_count <= 0) {
@@ -421,14 +436,15 @@ int crash_load_dm_crypt_keys(struct kimage *image)
}
if (!is_dm_key_reused) {
- image->dm_crypt_keys_addr = 0;
r = build_keys_header();
- if (r) {
- pr_err("Failed to build dm-crypt keys header, ret=%d\n", r);
- return r;
- }
+ if (r)
+ goto out;
}
+ /*
+ * keys_header will be copied to reserver memory later and then be
+ * cleaned up at the end of kexec_file_load syscall
+ */
kbuf.buffer = keys_header;
kbuf.bufsz = get_keys_header_size(key_count);
@@ -438,18 +454,33 @@ int crash_load_dm_crypt_keys(struct kimage *image)
r = kexec_add_buffer(&kbuf);
if (r) {
pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
- kvfree((void *)kbuf.buffer);
- return r;
+ goto out;
}
+
image->dm_crypt_keys_addr = kbuf.mem;
image->dm_crypt_keys_sz = kbuf.bufsz;
kexec_dprintk(
"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
kbuf.bufsz, kbuf.memsz);
+out:
+ is_dm_key_reused = false;
return r;
}
+void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
+{
+ /*
+ * For CPU/memory hot-plugging, the kdump image will be reloaded. Prevent
+ * keys_header from being cleaned up during unloading when
+ * is_dm_key_reused=true
+ */
+ 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]
- Sourabh Jain
And if that is the case, I think we should clean key_headers on kexec -u.
Most of the cases is_dm_key_reused is false thus keys_header will be
cleaned on kexec -u.
- Sourabh Jain
IIUC, for the case where CONFIG_CRASH_HOTPLUG is not enabled, this is how key
restore works:
After loading the kdump kernel for the first time, the state of the variables is:
is_dm_key_reused = false
keys_header = NULL
For example, if 2 CPUs are hot-removed and kdump is reloaded twice:
Then the sequence of operations needed to ensure the loaded keys can be reused is:
Udev rule triggered on the 1st CPU hotplug:
echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
Restore the key header from the reserved area
Reload the kdump service/kernel
Udev rule triggered on the 2nd CPU hotplug:
echo true > /sys/kernel/config/crash_dm_crypt_keys/reuse
Restore the key header from the reserved area
Reload the kdump service/kernel
What I don’t understand is the need to restore the key header from crashkernel
memory for every hotplug operation.
I think there is no easy way to tell if there is another hotplug and
considering hotplug is a rare event. So I guess it's OK to restore the
key headers for every hotplug operation.