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

[sashiko-bot@kernel.org]

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