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 <[email protected]>
> ---
> 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 <[email protected]>
>
> 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
>
