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 Sun, Aug 30, 2026 at 11:07:30AM +0530, Sourabh Jain wrote:
>
>
>On 29/08/26 17:47, Coiby Xu wrote:
>>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!
>
>No worries at all! Happy to help, and thanks for the patch.

Thank you:)

>
>
>>
>>>
>>>
>>>>+    }
>>>>+
>>>>+    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.
>
>Ah yes, I missed that. That said, we are calling
>kimage_file_post_load_cleanup() twice for every kexec_file_load, so this
>needs to be looked into separately.

Thanks for the reminder! I have a further look at
kimage_file_post_load_cleanup and can confirm repeatedly calling
kexec_file_post_load_cleanup_dm_crypt won't cause any trouble.

Btw, I think there is no need to call kimage_file_post_load_cleanup
twice because kimage_free is to be called for sure. I also notice
kimage_file_alloc_init also calls kimage_file_post_load_cleanup which
seems unnecessary as well. kimage_file_alloc_init even calls
kfree(image) which may lead to UAF. Anyways these are separate issues
thus shall be addressed in separate patches.


-- 
Best regards,
Coiby