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


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

>     // 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!

That will be good. Lock ownership will be with just one function, which 
will make
things easier to manage.

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