Re: [PATCH v5 4/9] crash_dump: Free temporary dm-crypt keys_header buffer in kdump kernel

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Mixing goto-based cleanup labels with __free() scope-based cleanup in the same function.

Pre-existing issues:
- [High] Out-of-bounds memory read using kmap_local_page() for a multi-page buffer.
--

commit a9106b1595e8efc92f366f113d95f7f3216942f5
Author: Coiby Xu <coiby.xu@xxxxxxxxx>

    crash_dump: Free temporary dm-crypt keys_header buffer in kdump kernel
    
    Although we expect the system to reboot immediately after vmcore dumping
    is finished, it's still good to free the temporary keys_header buffer.
    
    Fixes: 62f17d9df692 ("crash_dump: retrieve dm crypt keys in kdump kernel")
    Reported-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
    Reviewed-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
    Signed-off-by: Coiby Xu <coiby.xu@xxxxxxxxx>

> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 580534d43b66e..ff6055a97a4b6 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -118,6 +118,7 @@ static int get_keys_from_kdump_reserved_memory(void)
>  
>  static int restore_dm_crypt_keys_to_thread_keyring(void)
>  {
> +	struct keys_header *keys_header __free(kfree_sensitive) = NULL;

[Severity: Medium]
Does this code mix __free() scope-based cleanup with goto cleanup labels?

The function restore_dm_crypt_keys_to_thread_keyring() newly introduces
a __free() declaration for keys_header, but still contains a goto out block
at the end:

kernel/crash_dump_dm_crypt.c:restore_dm_crypt_keys_to_thread_keyring() {
    ...
out:
	key_ref_put(keyring_ref);
	return ret;
}

Mixing these two cleanup styles creates confusing ownership semantics and
increases the risk of double-frees or resource leaks when the code is
modified in the future.

[Severity: High]
This isn't a bug introduced by this patch, but does the adjacent function
get_keys_from_kdump_reserved_memory() read out-of-bounds?

It uses kmap_local_page() which only maps a single 4KB page, but memcpy()
copies keys_header_size bytes (which can be up to ~49KB) from it:

kernel/crash_dump_dm_crypt.c:get_keys_from_kdump_reserved_memory() {
    ...
	keys_header_loaded = kmap_local_page(pfn_to_page(
		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
    ...
	keys_header_size = get_keys_header_size(keys_header_loaded->total_keys);
    ...
	memcpy(keys_header, keys_header_loaded, keys_header_size);
    ...
}

On high memory systems (e.g., 32-bit architectures), this will trigger a
kernel page fault or read garbage data if the total size of the keys exceeds
what fits in a single page.

>  	struct dm_crypt_key *key;
>  	size_t keys_header_size;
>  	key_ref_t keyring_ref;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909003657.1570544-1-coiby.xu@xxxxxxxxx?part=4