Re: [PATCH v4 2/9] crash_dump: Disallow writing to dm-crypt configfs during kexec_file_load syscall

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 28/08/26 14:18, Coiby Xu wrote:
> If writing to the configfs group happens concurrently during
> kexec_file_load syscall, it may lead to the following issues,
>    - buffer overflow if dm-crypt keys are added after allocation
>    - stale total_keys if dm-crypt keys are removed during iteration
>    - keys_header will not be freed if config/crash_dm_crypt_key/reuse is
>      set true
>
> So hold config_keys_subsys.su_mutex for the entire sequence during the
> kexec_file_load syscall to ensure a consistent snapshot. To have serial
> access to config/crash_dm_crypt_key/reuse, use the kexec lock as we also
> need to access kexec_crash_image serially.
>
> Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump reserved memory")
> Suggested-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   kernel/crash_dump_dm_crypt.c | 28 ++++++++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 4335b6cb1fc4..026c7de4ad85 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -7,6 +7,7 @@
>   #include <linux/configfs.h>
>   #include <linux/module.h>
>   #include <linux/sysfs.h>
> +#include "kexec_internal.h"
>   
>   #define KEY_NUM_MAX 128	/* maximum dm crypt keys */
>   #define KEY_SIZE_MAX 256	/* maximum dm crypt key size */
> @@ -296,14 +297,20 @@ static ssize_t config_keys_reuse_store(struct config_item *item,
>   	bool val;
>   	int r;
>   
> +	if (!kexec_trylock()) {
> +		r = -EBUSY;
> +		goto unlock;


Are we unlocking a lock that we didn't acquire? How about returning 
-EBUSY directly instead?


> +	}
> +
> +	r = -EINVAL;
>   	if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {
>   		kexec_dprintk(
>   			"dm-crypt keys haven't be saved to crash-reserved memory\n");
> -		return -EINVAL;
> +		goto unlock;
>   	}
>   
>   	if (kstrtobool(page, &val) || !val)
> -		return -EINVAL;
> +		goto unlock;
>   
>   	if (is_dm_key_reused) {
>   		pr_info("Already got dm-crypt keys, please continue with kexec_file_load syscall\n");
> @@ -311,12 +318,15 @@ static ssize_t config_keys_reuse_store(struct config_item *item,
>   		r = get_keys_from_kdump_reserved_memory();
>   		if (r) {
>   			pr_warn("Failed to get dm-crypt keys from reserved memory\n");
> -			return r;
> +			goto unlock;
>   		}
>   		is_dm_key_reused = true;
[...]
>   	}
>   
> -	return count;
> +	r = count;
> +unlock:
> +	kexec_unlock();
> +	return r;
>   }
>   
>   CONFIGFS_ATTR(config_keys_, reuse);
> @@ -421,6 +431,8 @@ static int build_keys_header(void)
>   	return 0;
>   }
>   
> +static bool mutex_acquired;
> +
>   int crash_load_dm_crypt_keys(struct kimage *image)
>   {
>   	struct kexec_buf kbuf = {
> @@ -432,6 +444,9 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	};
>   	int r = 0;
>   
> +	mutex_lock(&config_keys_subsys.su_mutex);
> +	mutex_acquired = true;
> +
>   	if (key_count <= 0) {
>   		kexec_dprintk("No dm-crypt keys\n");
>   		return 0;
> @@ -481,6 +496,11 @@ void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)

I explored the kexec_file_load syscall entry path and noticed
that there is a path where the call to
kimage_file_post_load_cleanup() can be skipped.

For example, if kexec_file_load is called and everything goes well
but kexec_post_load() fails, we skip
kimage_file_post_load_cleanup(), and consequently
kexec_file_post_load_cleanup_dm_crypt() as well.

If that happens, the config_keys_subsys.su_mutex remains locked,
which could cause problems with the next kexec load, right?

- Sourabh Jain

>   		kfree_sensitive(keys_header);
>   		keys_header = NULL;
>   	}
> +
> +	if (mutex_acquired) {
> +		mutex_unlock(&config_keys_subsys.su_mutex);
> +		mutex_acquired = false;
> +	}
>   }
>   
>   static int __init configfs_dmcrypt_keys_init(void)