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

[Baoquan He <baoquan.he@linux.dev>]

On 07/04/26 at 07:25pm, 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(-)

LGTM,

Acked-by: Baoquan He <baoquan.he@linux.dev>

> 
> 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
>