[PATCH] module: fix UAF and GPF in idempotent_init_module via heap allocation

[PATCH] module: fix UAF and GPF in idempotent_init_module via heap allocation

[Mingyu Wang]

During concurrent module loading (e.g., triggered by syzkaller), the
idempotent module loading mechanism uses a local stack variable
(`struct idempotent idem`) to track the state of waiters.

If a task executing idempotent_init_module() is abruptly terminated
(e.g., killed by a fatal signal or otherwise completely exits before
reaching the list cleanup paths) after adding its node to the global
`idem_hash` list, its kernel stack is prematurely freed and reclaimed.

However, the stack-allocated node remains linked in the list. Subsequent
module loading attempts that traverse the list will dereference this
stale stack pointer, leading to KASAN slab-out-of-bounds reads and
General Protection Faults (GPF):

  BUG: KASAN: slab-out-of-bounds in idempotent_init_module+0x54a/0x620
  Read of size 8 at addr ffff888106367df8 by task modprobe/433
  ...
  The buggy address belongs to the object at ffff8881063676c0
   which belongs to the cache shmem_inode_cache of size 1392
  ...
  Oops: general protection fault, probably for non-canonical address

Fix this by dynamically allocating `struct idempotent` on the heap
via `kmalloc_obj()`. This decouples the list node's lifespan from the
process stack, ensuring that even if the task is abruptly terminated,
the global list safely points to valid heap memory until properly
unlinked, preventing memory corruption.

Fixes: 9b9879fc0327 ("modules: catch concurrent module loads, treat them as idempotent")
Cc: stable@vger.kernel.org
Signed-off-by: Mingyu Wang <25181214217@stu.xidian.edu.cn>
---
 kernel/module/main.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 46dd8d25a605..39f05ac4b1a1 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3779,21 +3779,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;
+
 	/* 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;
 }
 
 SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
-- 
2.34.1

Re: [PATCH] module: fix UAF and GPF in idempotent_init_module via heap allocation

[sashiko-bot]

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 <25181214217@stu.xidian.edu.cn>

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/20260717082601.81704-1-25181214217@stu.xidian.edu.cn?part=1