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

On 7/17/26 10:26 AM, Mingyu Wang wrote:
> 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.

I'm confused by this description and I'm not sure what problem the patch
is trying to solve. A task is not normally terminated in the middle of
a syscall. This could happen only if it hits an oops. However, that
means there is an earlier bug and the module loader isn't expected to be
resilient to a post-oops state.

Could you clarify the scenario that you ran into?

-- 
Thanks,
Petr

> 
> 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: [email protected]
> Signed-off-by: Mingyu Wang <[email protected]>
> ---
>  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)