Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [Medium] Memory and reference leak of the key returned by key_create_or_update() on the success path. - [Medium] Incorrect return value assignment from `dm_crypt_keys_read()` breaks the caller's success check for empty key sets. Pre-existing issues: - [High] Missing validation of `dm_crypt_keys_addr` leads to arbitrary memory reads. - [High] Buffer over-read from single mapped page in get_keys_from_kdump_reserved_memory(). - [High] Missing validation on parsed key attributes leads to massive Heap Out-of-Bounds reads and Information Leak. - [Medium] Double fetch of the key count from untrusted memory leads to Heap Out-of-Bounds read. -- commit e9dcb18898d315e8ec7102932e9391f9c0892b97 Author: Coiby Xu <[email protected]> Subject: crash_dump: Check the function return codes in restore_dm_crypt_keys_to_thread_keyring This commit adds return code checking for dm_crypt_keys_read() and add_key_to_keyring() to properly abort when reading old memory or allocating a keyring fails. It also deletes unused code related to the key reference. > diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c > index b8fe630462cfa..bc70fbb79e0f6 100644 > --- a/kernel/crash_dump_dm_crypt.c > +++ b/kernel/crash_dump_dm_crypt.c > @@ -66,7 +66,7 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key, > key_ref_t keyring_ref) > { > key_ref_t key_ref; > - int r; > + int r = 0; > > /* create or update the requested key and add it to the target keyring > */ > key_ref = key_create_or_update(keyring_ref, "user", dm_key->key_desc, > @@ -74,8 +74,6 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key, > KEY_USR_ALL, KEY_ALLOC_IN_QUOTA); > > if (!IS_ERR(key_ref)) { > - r = key_ref_to_ptr(key_ref)->serial; > - key_ref_put(key_ref); > pr_debug("Success adding key %s\n", dm_key->key_desc); [Severity: Medium] Does removing key_ref_put() here leak the key reference? key_create_or_update() returns a key reference specifically for the caller with an incremented usage count, so without key_ref_put(), the key's usage count might be permanently elevated, preventing it from ever being garbage collected. > } else { > r = PTR_ERR(key_ref); [ ... ] > @@ -134,9 +132,14 @@ static int restore_dm_crypt_keys_to_thread_keyring(void) > } > > addr = dm_crypt_keys_addr; > - dm_crypt_keys_read((char *)&key_count, sizeof(key_count), &addr); > + ret = dm_crypt_keys_read((char *)&key_count, sizeof(key_count), &addr); [Severity: High] This is a pre-existing issue, but is it safe to read from dm_crypt_keys_addr without verifying if it was ever initialized by the command-line argument? If the kdump kernel is booted without the dmcryptkeys= parameter, dm_crypt_keys_addr might remain 0, leading to arbitrary memory reads when this sysfs attribute is triggered. > + if (ret < 0) { > + pr_err("Failed to read the number of dm-crypt keys\n"); > + goto out; > + } > + [ ... ] > @@ -151,12 +154,18 @@ static int restore_dm_crypt_keys_to_thread_keyring(void) > goto out; > } > > - dm_crypt_keys_read((char *)keys_header, keys_header_size, &addr); > + ret = dm_crypt_keys_read((char *)keys_header, keys_header_size, &addr); [Severity: Medium] If dm_crypt_keys_read() succeeds, it returns a positive byte count which is assigned to ret. If there are no keys and the loop below doesn't execute, will this function incorrectly return a positive value instead of 0? The caller config_keys_restore_store() expects !ret to set restore = true, which would fail if ret is a positive byte count. > + if (ret < 0) { > + pr_err("Failed to read dm-crypt keys\n"); > + goto out; > + } > > for (int i = 0; i < keys_header->total_keys; i++) { [Severity: Medium] This is a pre-existing issue, but does the second read from oldmem physically overwrite keys_header->total_keys with unvalidated data? If oldmem changes concurrently or the read partially fails, the newly fetched total_keys can exceed the originally validated key_count. The loop bounds check uses this unvalidated bound, potentially causing a heap out-of-bounds read. > key = &keys_header->keys[i]; > pr_debug("Get key (size=%u)\n", key->key_size); > - add_key_to_keyring(key, keyring_ref); > + ret = add_key_to_keyring(key, keyring_ref); [Severity: High] This isn't a bug introduced by this patch, but are key->key_size and key->key_desc validated before being passed to add_key_to_keyring()? The completely untrusted key_size is passed as the payload length to key_create_or_update(), which might read far out-of-bounds from dm_key->data and embed leaked kernel heap memory into the key payload. Additionally, key_desc is not checked for null-termination. > + if (ret) > + break; > } [ ... ] While reviewing this file, I also noticed another pre-existing issue in get_keys_from_kdump_reserved_memory(): 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)); ... memcpy(keys_header, keys_header_loaded, keys_header_size); ... } [Severity: High] This isn't a bug introduced by this patch, but does this memcpy() read past the boundaries of the mapped page? kmap_local_page() establishes a mapping for exactly one page (4096 bytes), but keys_header_size could be up to ~49KB. This will read far beyond the single page boundary, potentially causing a kernel panic on architectures where kmap_local_page() does not fall back to the direct map. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
