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

[Coiby Xu]

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

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