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

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

[Guangshuo Li]

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.

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>
---
 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 a20d4097744a..641c290f1270 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -80,7 +80,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)
 	size_t keys_header_size;
 	key_ref_t keyring_ref;
 	u64 addr;
+	int ret = 0;
 
 	/* find the target keyring (which must be writable) */
 	keyring_ref =
@@ -117,7 +117,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 < 0 || 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);
@@ -125,8 +126,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);
 
@@ -136,7 +139,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_keying(struct dm_crypt_key *dm_key)
-- 
2.43.0

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

[Coiby Xu]

Hi Guangshuo,

Thanks for sending this patch! Your fix is more complete than my version
https://lore.kernel.org/kexec/20260501234342.2518281-2-coiby.xu@gmail.com/
So I plan to drop mine from the patch set. I only have some nitpicking
for this patch. Please check inline comments.

On Wed, Jun 03, 2026 at 09:50:56PM +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.

My testing shows when there are more than five keys to be added, this
"refcount_t: underflow; use-after" error can occur. Maybe you can
include this info in your commit msg.

>
>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>
>---
> 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 a20d4097744a..641c290f1270 100644
>--- a/kernel/crash_dump_dm_crypt.c
>+++ b/kernel/crash_dump_dm_crypt.c
>@@ -80,7 +80,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)
> 	size_t keys_header_size;
> 	key_ref_t keyring_ref;

I think ordering local variables from longest line length to shortest line
length a.k.a Reverse Christmas Tree style is preferred i.e.
     int ret = 0;
     u64 addr;


-- 
Best regards,
Coiby

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

[Guangshuo Li]

Hi Coiby,

Thanks for the review and for sharing the testing result.

On Thu, 25 Jun 2026 at 18:50, Coiby Xu <coiby.xu@gmail.com> wrote:
>
> Hi Guangshuo,
>
> Thanks for sending this patch! Your fix is more complete than my version
> https://lore.kernel.org/kexec/20260501234342.2518281-2-coiby.xu@gmail.com/
> So I plan to drop mine from the patch set. I only have some nitpicking
> for this patch. Please check inline comments.
>
> On Wed, Jun 03, 2026 at 09:50:56PM +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.
>
> My testing shows when there are more than five keys to be added, this
> "refcount_t: underflow; use-after" error can occur. Maybe you can
> include this info in your commit msg.
>
> >
> >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>
> >---
> > 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 a20d4097744a..641c290f1270 100644
> >--- a/kernel/crash_dump_dm_crypt.c
> >+++ b/kernel/crash_dump_dm_crypt.c
> >@@ -80,7 +80,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)
> >       size_t keys_header_size;
> >       key_ref_t keyring_ref;
>
> I think ordering local variables from longest line length to shortest line
> length a.k.a Reverse Christmas Tree style is preferred i.e.
>      int ret = 0;
>      u64 addr;
>
>
> --
> Best regards,
> Coiby

I have sent v2 with the commit message updated to mention the refcount
underflow/use-after-free warning, and also fixed the local variable
ordering as suggested.

Thanks,
Guangshuo