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

[sashiko-bot@kernel.org]

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.

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

commit 9cf0538566bf6406e97a7f29629856abff44a5b0
Author: Coiby Xu <coiby.xu@gmail.com>

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@gmail.com?part=1