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

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

On 02/05/26 05:13, 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.


Yes, this will solve many synchronization problems when a  user tries to
perform any operation under our configfs_subsystem while the kernel is
executing the kexec_file_load system call.

Now, regarding the third point about freeing key_header: this will work
only if configfs takes the su_mutex lock before invoking the store callback.
I am not sure whether it actually does.

However, based on my previous comment on (2/9), if we keep 
config_keys_reuse_store()
under the kexec lock, this will be taken care of. This is because the entire
kexec_file_load system call already runs under the kexec lock.


>
> Fixes: 479e58549b0f ("crash_dump: store dm crypt keys in kdump reserved memory")
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>   kernel/crash_dump_dm_crypt.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index 4d8a3331bbe7..6377ee86ec50 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -429,6 +429,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>   	};
>   	int r = 0;
>   
> +	mutex_lock(&config_keys_subsys.su_mutex);
>   
>   	if (key_count <= 0) {
>   		kexec_dprintk("No dm-crypt keys\n");
> @@ -479,6 +480,9 @@ void kexec_file_post_load_cleanup_dm_crypt(struct kimage *image)
>   		kfree_sensitive(keys_header);
>   		keys_header = NULL;
>   	}
> +
> +	if (mutex_is_locked(&config_keys_subsys.su_mutex))
> +		mutex_unlock(&config_keys_subsys.su_mutex);
How about release the lock in crash_load_dm_crypt_keys() itself? Given 
that config_keys_reuse_store() is placed under kexec lock?


- Sourabh Jain

>   }
>   
>   static int __init configfs_dmcrypt_keys_init(void)