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 <[email protected]> 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 <[email protected]> Reviewed-by: Sourabh Jain <[email protected]> Signed-off-by: Coiby Xu <[email protected]> > 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/[email protected]?part=4
