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

[Coiby Xu]

On Wed, Sep 09, 2026 at 12:53:11AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Prematurely clearing `is_dm_key_reused` breaks the ARM64 kexec_file_load retry loop and defeats hotplug dm-crypt key reuse.

Thanks for catching this issue! I'll send a new version to address this
issue.

> Pre-existing issues:
> - [High] Mapping crash-reserved memory with `kmap_local_page()` causes out-of-bounds reads if dm-crypt keys exceed a single page.

x86 is not supported so I think this concern can be dismissed.

> --
>
> commit 9cf0538566bf6406e97a7f29629856abff44a5b0
> Author: Coiby Xu <coiby.xu@xxxxxxxxx>
>
> crash_dump: Fix potential double free and UAF of keys_header
>
> This commit addresses a potential double free and UAF of keys_header when
> loading dm-crypt keys for kdump. It establishes an invariant to free
> keys_header at the end of kexec_file_load unless reuse=true.
>
> > diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
> > index b70f4df15a1ae..1a0ca54adc9a4 100644
> > --- a/arch/arm64/kernel/kexec_image.c
> > +++ b/arch/arm64/kernel/kexec_image.c
> > @@ -110,6 +110,7 @@ static void *image_load(struct kimage *image,
> >  		image->nr_segments -= 1;
> >  		kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz;
> >  		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> > +		crash_dm_crypt_cleanup(image);
>
> [Severity: High]
> Does this call to crash_dm_crypt_cleanup() during a retry loop unintentionally
> free the keys_header buffer?
>
> Since crash_load_dm_crypt_keys() unconditionally clears the is_dm_key_reused
> flag before returning, any mid-retry cleanup performed here when segment
> placement fails will see the flag as false and wrongfully free the valid
> keys_header buffer.
>
> >  	}
> >
> >  	if (ret) {
>
> [ ... ]
>
> > diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> > index c685497cd470e..f638fb4b8554c 100644
> > --- a/kernel/crash_dump_dm_crypt.c
> > +++ b/kernel/crash_dump_dm_crypt.c
> > @@ -84,18 +84,25 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
>
> [ ... ]
>
> > -static void get_keys_from_kdump_reserved_memory(void)
> > +static int get_keys_from_kdump_reserved_memory(void)
> >  {
> >  	struct keys_header *keys_header_loaded;
> > +	size_t keys_header_size;
> >
> > -	arch_kexec_unprotect_crashkres();
> > +	keys_header_size = get_keys_header_size(key_count);
> > +	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
> > +	if (!keys_header)
> > +		return -ENOMEM;
> >
> > +	arch_kexec_unprotect_crashkres();
> >  	keys_header_loaded = kmap_local_page(pfn_to_page(
> >  		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
> >
> > -	memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
> > +	memcpy(keys_header, keys_header_loaded, keys_header_size);
>
> [Severity: High]
> This is a pre-existing issue, but does this copy read out of bounds?
>
> kmap_local_page() guarantees mapping exactly one physical page frame. If user
> configuration has enough dm-crypt keys (e.g., > 10 keys, where each struct is
> 388 bytes), keys_header_size easily exceeds the 4KB page size.
>
> Copying beyond the single-page mapping will cause out-of-bounds reads and
> could trigger a kernel panic on systems with HIGHMEM or when
> CONFIG_DEBUG_KMAP_LOCAL is enabled. Could memremap() be used instead?
>
> >  	kunmap_local(keys_header_loaded);
> >  	arch_kexec_protect_crashkres();
> > +
> > +	return 0;
> >  }
>
> [ ... ]
>
> > @@ -441,18 +458,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;
>
> [Severity: High]
> Does prematurely clearing is_dm_key_reused here destroy the state needed for
> the segment placement retry loop?
>
> On ARM64, if the first attempt at segment placement fails, the kexec load
> code retries the loop. Since the flag is unconditionally cleared here, the
> first iteration will clear it. When the retry cleanup executes in
> image_load(), it will see is_dm_key_reused == false, freeing the keys_header
> buffer and leaving subsequent retries without the required keys.
>
> >  	return r;
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260909003657.1570544-1-coiby.xu@xxxxxxxxx?part=1

--
Best regards,
Coiby