Re: [PATCH v2] crash_dump: release keyring reference at the correct time

[Coiby Xu <coiby.xu@gmail.com>]

On Sat, Jul 04, 2026 at 07:25:09PM +0800, Guangshuo Li wrote:
>restore_dm_crypt_keys_to_thread_keyring() gets a reference to the user
>keyring before restoring the saved dm-crypt keys.
>
>The same keyring reference is then passed to add_key_to_keyring() for each
>saved key, but add_key_to_keyring() drops that reference on every call.
>This is only balanced when exactly one key is restored. With multiple
>keys, the keyring reference is dropped too many times and may trigger a
>refcount underflow or use-after-free.
>
>When more than five keys are restored, a refcount underflow/use-after-free
>warning can be triggered.
>
>The early error paths after lookup_user_key() also return without dropping
>the keyring reference.
>
>Keep ownership of the keyring reference in
>restore_dm_crypt_keys_to_thread_keyring(), drop it once on all exit paths,
>and make add_key_to_keyring() only use the reference without consuming it.
>
>Fixes: 62f17d9df692 ("crash_dump: retrieve dm crypt keys in kdump kernel")
>Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>---
>v2:
>  - Mention the refcount underflow/use-after warning seen when restoring
>    more than five keys, as suggested by Coiby Xu.
>  - Order local variables in reverse Christmas tree style.
>
> kernel/crash_dump_dm_crypt.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
>diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>index cb875ddb6ba6..c685497cd470 100644
>--- a/kernel/crash_dump_dm_crypt.c
>+++ b/kernel/crash_dump_dm_crypt.c
>@@ -81,7 +81,6 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
> 		kexec_dprintk("Error when adding key");
> 	}
>
>-	key_ref_put(keyring_ref);
> 	return r;
> }
>
>@@ -104,6 +103,7 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
> 	struct dm_crypt_key *key;
> 	size_t keys_header_size;
> 	key_ref_t keyring_ref;
>+	int ret = 0;
> 	u64 addr;
>
> 	/* find the target keyring (which must be writable) */
>@@ -118,7 +118,8 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
> 	dm_crypt_keys_read((char *)&key_count, sizeof(key_count), &addr);
> 	if (key_count > KEY_NUM_MAX) {
> 		kexec_dprintk("Failed to read the number of dm-crypt keys\n");
>-		return -1;
>+		ret = -1;
>+		goto out;
> 	}
>
> 	kexec_dprintk("There are %u keys\n", key_count);
>@@ -126,8 +127,10 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
>
> 	keys_header_size = get_keys_header_size(key_count);
> 	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
>-	if (!keys_header)
>-		return -ENOMEM;
>+	if (!keys_header) {
>+		ret = -ENOMEM;
>+		goto out;
>+	}
>
> 	dm_crypt_keys_read((char *)keys_header, keys_header_size, &addr);
>
>@@ -137,7 +140,9 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
> 		add_key_to_keyring(key, keyring_ref);
> 	}
>
>-	return 0;
>+out:
>+	key_ref_put(keyring_ref);
>+	return ret;
> }
>
> static int read_key_from_user_keyring(struct dm_crypt_key *dm_key)
>-- 
>2.43.0
>
>

Hi Guangshuo,

Thanks for addressing my feedback!

Reviewed-and-tested-by: Coiby Xu <Coiby.Xu@gmail.com>

-- 
Best regards,
Coiby