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

[Coiby Xu <coiby.xu@gmail.com>]

On Wed, Sep 09, 2026 at 12:53:11AM +0000, sashiko-bot@kernel.org 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@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

-- 
Best regards,
Coiby