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

[Sourabh Jain]

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@xxxxxxxxxxxxx>
> > > Signed-off-by: Coiby Xu <coiby.xu@xxxxxxxxx>
> > > ---
> > >  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)