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

[Coiby Xu <coiby.xu@gmail.com>]

On Sat, Aug 29, 2026 at 12:32:46PM +0530, Sourabh Jain wrote:
>
>
>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?

Ah, thanks for catching my mistake! And also thanks for prioritizing
reviewing my patch!

>
>
>>+	}
>>+
>>+	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?

Thanks for exploring the kexec_file_load syscall entry path! I took a
further look at it. Even though kimage_file_post_load_cleanup can be
skipped, kimage_free will be called regardless which will in turn call
kimage_file_post_load_cleanup. 

     // kernel/kexec_core.c
     void kimage_free(struct kimage *image)
     {
     	/*
     	 * Free up any temporary buffers allocated. This might hit if
     	 * error occurred much later after buffer allocation.
     	 */
     	if (image->file_mode)
     		kimage_file_post_load_cleanup(image);
     }

But I realize by using kexec lock to ensure serial access to
is_dm_key_reused, we can actually release the mutex lock at the end of
crash_load_dm_crypt_keys function which can make the code simpler and
potentially more robust. Thanks for inspiring me to find a better
way!

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

-- 
Best regards,
Coiby