Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Severe locking imbalance in `crash_load_dm_crypt_keys()` leads to a leaked mutex on the error path and unlocking an unacquired mutex on the success path. Pre-existing issues: - [High] Premature and unconditional reset of `is_dm_key_reused` to false breaks ARM64/LoongArch kexec load retries and defeats intended cleanup logic. -- commit a9405cf8948ba39213612c61a1f3b10b23f437e4 Author: Coiby Xu <[email protected]> crash_dump: Disallow writing to dm-crypt configfs during kexec_file_load syscall This commit prevents concurrent writes to the configfs group during the kexec_file_load syscall by utilizing config_keys_subsys.su_mutex. It aims to avoid buffer overflows, stale keys, or memory leaks caused by changes to dm-crypt keys during iteration. > diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c > index 3a416933979f8..580534d43b66e 100644 > --- a/kernel/crash_dump_dm_crypt.c > +++ b/kernel/crash_dump_dm_crypt.c [ ... ] > @@ -478,6 +490,9 @@ int crash_load_dm_crypt_keys(struct kimage *image) > "Loaded dm crypt keys to kexec_buffer bufsz=0x%lx > memsz=0x%lx\n", > kbuf.bufsz, kbuf.memsz); > > +unlock: > + mutex_unlock(&config_keys_subsys.su_mutex); > + [Severity: High] Looking at crash_load_dm_crypt_keys(), if kexec_add_buffer() fails earlier in the function, it jumps to the out label, bypassing this unlock label. Does this leak the config_keys_subsys.su_mutex if the lock was acquired? Also, on the success path, mutex_unlock() is executed unconditionally. If is_dm_key_reused was true at the top of the function, the mutex was never acquired. Can this cause a kernel panic or corruption by unlocking an unacquired mutex? > out: > - return r; > + is_dm_key_reused = false; > + return r; > } [Severity: High] This is a pre-existing issue, but since the error and success paths unconditionally reset is_dm_key_reused to false here, does this break architectures like ARM64 that might retry segment loading? If load_other_segments() retries the load sequence, the second pass through crash_load_dm_crypt_keys() will see is_dm_key_reused as false, find zero keys, and silently load nothing into the kdump kernel. Additionally, crash_dm_crypt_cleanup() checks !is_dm_key_reused to determine whether it should free keys_header. Since the flag is unconditionally cleared here first, does this defeat the intended cleanup protection logic? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
