Re: [PATCH v15 09/23] kexec: Fix UAF and Double Free in crash_load_dm_crypt_keys()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch breaks the `is_dm_key_reused` feature by erroneously discarding recovered keys and silently falling back to the user keyring on kexec placement retries.

Pre-existing issues:
- [Critical] A severe heap buffer overflow occurs in `get_keys_from_kdump_reserved_memory()` due to the allocation size of `keys_header` becoming desynchronized with a dynamically updated `key_count`.
- [Critical] A Time-Of-Check to Time-Of-Use race between reading the global `key_count` and iterating the unlocked configfs list allows a heap buffer overflow when populating `keys_header`.
- [High] A race condition exists between the configfs `reuse` attribute store and the `kexec_file_load` syscall, allowing a Use-After-Free dereference of the `kexec_crash_image` pointer.
- [High] The `build_keys_header` function iterates over the configfs `cg_children` list without holding the required subsystem mutex, leading to a Use-After-Free or list corruption.
- [High] The `config_keys_restore_store` function lacks synchronization and unconditionally reallocates the global `keys_header` when toggled, causing a memory leak.
--

commit 5c3cfda384c3afc1b8e6d5155bf696ea37a80c9b
Author: Jinjie Ruan <ruanjinjie@huawei.com>

kexec: Fix UAF and Double Free in crash_load_dm_crypt_keys()

This commit attempts to fix a use-after-free and double free in the
dm-crypt keys handling path during arm64 kexec image placement retries.
It zeroes the keys_header global pointer after it is freed in the failure
path and updates the header regeneration check to trigger a fresh build
if keys_header is missing.

> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index cb875ddb6ba68..2c54628763377 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -412,13 +412,12 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>  	};
>  	int r;
>  
> -
>  	if (key_count <= 0) {
>  		kexec_dprintk("No dm-crypt keys\n");
>  		return 0;
>  	}
>  
> -	if (!is_dm_key_reused) {
> +	if (!is_dm_key_reused || unlikely(!keys_header)) {
[Severity: High]
If kexec_add_buffer() fails and we retry, does this new condition
unintentionally discard the recovered keys?

When is_dm_key_reused is true, falling back to build_keys_header() here
appears to overwrite the intended configuration by fetching keys from the
current user keyring instead of reusing the recovered dm-crypt keys.

>  		image->dm_crypt_keys_addr = 0;
>  		r = build_keys_header();
>  		if (r) {
[ ... ]
> @@ -437,6 +436,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>  	if (r) {
>  		pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
>  		kvfree((void *)kbuf.buffer);
> +		keys_header = NULL;
[Severity: High]
Nulling the pointer here triggers the faulty fallback mentioned above on
the next iteration of the retry loop.

>  		return r;
>  	}
>  	image->dm_crypt_keys_addr = kbuf.mem;

[Severity: Critical]
This is a pre-existing issue, but is there a risk of a heap buffer overflow
during reuse?

kernel/crash_dump_dm_crypt.c:get_keys_from_kdump_reserved_memory() {
    ...
    memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
    ...
}

If a user adds more keys via configfs after the initial allocation, key_count
increments. When reuse is triggered, this copies data based on the new
key_count into a keys_header buffer that was sized for the old key_count,
writing past the allocated bounds.

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free?

kernel/crash_dump_dm_crypt.c:config_keys_reuse_store() {
    ...
    if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {
    ...
}

Since kexec_crash_image is checked without synchronization, a concurrent
kexec_file_load syscall could swap and free the old image. The configfs
thread might then dereference the freed pointer when calculating the page
frame in get_keys_from_kdump_reserved_memory().

[Severity: High]
This is a pre-existing issue, but does this loop traverse the configfs list
unsafely?

kernel/crash_dump_dm_crypt.c:build_keys_header() {
    ...
    list_for_each_entry(item, &config_keys_subsys.su_group.cg_children,
                        ci_entry) {
    ...
}

The cg_children list is dynamically modified via mkdir/rmdir and is normally
protected by su_mutex. Since this runs during kexec load without that lock,
a concurrent rmdir could mutate the list, leading to list corruption or a
use-after-free.

[Severity: Critical]
This is a pre-existing issue, but is there a time-of-check to time-of-use
race that causes an out-of-bounds write?

kernel/crash_dump_dm_crypt.c:build_keys_header() {
    ...
    keys_header = kzalloc(get_keys_header_size(key_count), GFP_KERNEL);
    ...
    list_for_each_entry(item, &config_keys_subsys.su_group.cg_children,
                        ci_entry) {
        ...
        r = read_key_from_user_keyring(&keys_header->keys[i]);
    ...
}

If a concurrent process adds a new configfs key after the allocation but
during the loop, the unlocked traversal will iterate over more items than
allocated. Without bounds checking against the original key_count, this
overflows the keys array.

[Severity: High]
This is a pre-existing issue, but does this unconditionally leak memory?

kernel/crash_dump_dm_crypt.c:config_keys_restore_store() {
    ...
    if (!restore)
        restore_dm_crypt_keys_to_thread_keyring();
    ...
}

kernel/crash_dump_dm_crypt.c:restore_dm_crypt_keys_to_thread_keyring() {
    ...
    keys_header = kzalloc(keys_header_size, GFP_KERNEL);
    ...
}

There is no check if keys_header is already allocated, nor is the existing
pointer freed. A user could repeatedly write '0' to the restore attribute
to trigger the allocation and exhaust memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601094805.2928614-1-ruanjinjie@huawei.com?part=9