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

[Coiby Xu]

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

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