Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Unconditionally clearing `is_dm_key_reused` defeats the key caching mechanism, silently breaking hotplug kdump reloads and ARM64 segment retries. Pre-existing issues: - [High] Out-of-bounds read past the 4KB window of `kmap_local_page` when reading dm-crypt keys from reserved memory. -- commit f9061da466f53a4935cffc1bfaa1ac6f1ef0c0ef Author: Coiby Xu <[email protected]> crash_dump: Read the number of dm-crypt keys from reserved memory This patch reads the number of dm-crypt keys from reserved memory to handle cases where users add or delete keys by mistake. This aims to improve safety over reusing saved keys for CPU/memory hot-plugging. > diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c > index f638fb4b8554c..3a416933979f8 100644 > --- a/kernel/crash_dump_dm_crypt.c > +++ b/kernel/crash_dump_dm_crypt.c > @@ -88,21 +88,31 @@ static int get_keys_from_kdump_reserved_memory(void) > { > struct keys_header *keys_header_loaded; > size_t keys_header_size; > - > - keys_header_size = get_keys_header_size(key_count); > - keys_header = kzalloc(keys_header_size, GFP_KERNEL); > - if (!keys_header) > - return -ENOMEM; > + int r = 0; > > arch_kexec_unprotect_crashkres(); > keys_header_loaded = kmap_local_page(pfn_to_page( > kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT)); > > + if (keys_header_loaded->total_keys <= 0 || > + keys_header_loaded->total_keys > KEY_NUM_MAX) { > + pr_warn("keys_header saved to reserved memory may be > corrupt\n"); > + r = -EINVAL; > + goto kunmap; > + } > + > + keys_header_size = get_keys_header_size(keys_header_loaded->total_keys); > + keys_header = kzalloc(keys_header_size, GFP_KERNEL); > + if (!keys_header) { > + r = -ENOMEM; > + goto kunmap; > + } > + > memcpy(keys_header, keys_header_loaded, keys_header_size); [Severity: High] This is a pre-existing issue, but does this memcpy() cause an out-of-bounds read in get_keys_from_kdump_reserved_memory()? The pointer keys_header_loaded is set up using kmap_local_page(), which establishes a virtual mapping strictly limited to a single 4KB page. However, keys_header_size can reach up to approximately 49.6KB (128 keys at 388 bytes each). Could this read past the kmap_local_page window if enough keys are populated to exceed the 4KB boundary, causing a kernel page fault or copying unrelated memory into the dm-crypt keys buffer? > +kunmap: > kunmap_local(keys_header_loaded); > arch_kexec_protect_crashkres(); > - > - return 0; > + return r; > } [ ... ] > @@ -432,12 +442,13 @@ int crash_load_dm_crypt_keys(struct kimage *image) > }; > int r = 0; > > - if (key_count <= 0) { > - kexec_dprintk("No dm-crypt keys\n"); > - return 0; > - } > - > if (!is_dm_key_reused) { > + if (key_count <= 0) { > + kexec_dprintk("No dm-crypt keys\n"); > + r = 0; > + goto out; > + } > + [Severity: High] Does this goto out path in crash_load_dm_crypt_keys() unintentionally defeat the key caching mechanism? By jumping to the out label where is_dm_key_reused is set to false, it appears the kexec cleanup function (crash_dm_crypt_cleanup) will now unconditionally free keys_header since the flag is cleared. On architectures like ARM64, if kexec segment allocation fails and retries, the dm-crypt keys may be silently omitted from the new kdump image because the retry loop will skip loading them (returning success but leaving out the keys segment). Could this break the intended caching mechanism for CPU and memory hotplug events, leading to a silent failure to unlock the encrypted dump target during a crash? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
