Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] The patch fails to fix the root cause of the KASAN GPF and leaves a permanent deadlock in idempotent module loading. -- --- Patch [1]: [PATCH] module: fix UAF and GPF in idempotent_init_module via heap allocation --- commit e69cf661c8a6752503caa8d7d7d8b55a7d925049 Author: Mingyu Wang <[email protected]> module: fix UAF and GPF in idempotent_init_module via heap allocation This commit changes the local stack variable used for tracking idempotent module loading state to a heap allocation. It aims to prevent a KASAN slab-out-of-bounds read and general protection fault when a task exits abruptly while its node is still in the global hash list. > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 528690ba160bd..0cf66a911fb69 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -3780,21 +3780,29 @@ static int init_module_from_file(struct file *f, > const char __user * uargs, int > > static int idempotent_init_module(struct file *f, const char __user * uargs, > int flags) > { > - struct idempotent idem; > + struct idempotent *idem; > + int ret; > > if (!(f->f_mode & FMODE_READ)) > return -EBADF; > > + idem = kmalloc_obj(*idem, GFP_KERNEL); > + if (!idem) > + return -ENOMEM; > + [Severity: Critical] This is a pre-existing issue, but does moving this allocation to the heap actually resolve the crash, or does it just shift the uninitialized memory issue from the stack to the heap? If another task has already started loading the module, idempotent() finds the existing cookie and returns false without adding the new task's entry to the hash list. The loser task then goes to the else block below and calls idempotent_wait_for_completion(idem). Since it was never added to the list, the winner task's idempotent_complete() will never wake it up, resulting in a permanent deadlock. Furthermore, kmalloc_obj() with GFP_KERNEL does not zero the memory. If the deadlocked task is interrupted by a signal, idempotent_wait_for_completion() executes this cleanup path: kernel/module/main.c:idempotent_wait_for_completion() if (wait_for_completion_interruptible(&u->complete)) { spin_lock(&idem_lock); if (!hlist_unhashed(&u->entry)) hlist_del(&u->entry); Since the memory is uninitialized, hlist_unhashed() evaluates garbage data. If it evaluates to false, hlist_del() will dereference invalid pointers and cause memory corruption or a general protection fault. Would it make sense to zero-initialize the allocation to avoid evaluating garbage data, and ensure the loser's entry is properly linked before waiting so it can actually be woken up? > /* Are we the winners of the race and get to do this? */ > - if (!idempotent(&idem, file_inode(f))) { > - int ret = init_module_from_file(f, uargs, flags); > - return idempotent_complete(&idem, ret); > + if (!idempotent(idem, file_inode(f))) { > + ret = init_module_from_file(f, uargs, flags); > + ret = idempotent_complete(idem, ret); > + } else { > + /* > + * Somebody else won the race and is loading the module. > + */ > + ret = idempotent_wait_for_completion(idem); > } > > - /* > - * Somebody else won the race and is loading the module. > - */ > - return idempotent_wait_for_completion(&idem); > + kfree(idem); > + return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
