Re: [PATCH v3 1/6] module/dups: Inform duplicate requests about the result directly

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Re-arming `delete_work` while it is currently executing leads to a Use-After-Free and timer corruption.
- [High] Waiters sleep in `wait_for_completion_state()` without holding a reference to `kmod_req`, allowing `delete_work` to free the structure out from under them.
--

--- Patch [1]: [PATCH v3 1/6] module/dups: Inform duplicate requests about the result directly ---
commit b996b7173ceb4926f129328065b0224c2ccfe2d2
Author: Petr Pavlu <[email protected]>

module/dups: Inform duplicate requests about the result directly

This patch removes the complete_work workqueue item that was used to delay
the completion signal. It updates kmod_dup_request_announce() to call
complete_all() and then immediately queue delete_work to clean up the entry.

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 8b6f8f177de4f..45080f451e5c6 100644
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c
[ ... ]
> @@ -230,17 +205,17 @@ void kmod_dup_request_announce(char *module_name, int ret)
>  
>  	kmod_req->dup_ret = ret;
>  
> +	/* Inform all duplicate waiters to check the return value. */
> +	complete_all(&kmod_req->first_req_done);
> +

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free if a waiter thread is severely delayed after being woken up?

Waiters in kmod_dup_request_exists_wait() wait on this completion without
taking a reference to kmod_req:

kernel/module/dups.c:kmod_dup_request_exists_wait() {
    ...
    wait_for_completion_state(&kmod_req->first_req_done, ...);
    ...
    *dup_ret = kmod_req->dup_ret;
    ...
}

If a woken waiter is preempted, heavily throttled, or the system is suspended
for more than 60 seconds before it can return and read kmod_req->dup_ret,
delete_work will run and free kmod_req, causing a use-after-free when the
waiter resumes.

>  	/*
> -	 * If we complete() here we may allow duplicate threads
> -	 * to continue before the first one that submitted the
> -	 * request. We're in no rush also, given that each and
> -	 * every bounce back to userspace is slow we avoid that
> -	 * with a slight delay here. So queueue up the completion
> -	 * and let duplicates suffer, just wait a tad bit longer.
> -	 * There is no rush. But we also don't want to hold the
> -	 * caller up forever or introduce any boot delays.
> +	 * Now that we have allowed prior request_module() calls to go on
> +	 * with life, let's schedule deleting this entry. We don't have
> +	 * to do it right away, but we *eventually* want to do it so to not
> +	 * let this linger forever as this is just a boot optimization for
> +	 * possible abuses of vmalloc() incurred by finit_module() thrashing.
>  	 */
> -	queue_work(system_dfl_wq, &kmod_req->complete_work);
> +	queue_delayed_work(system_dfl_wq, &kmod_req->delete_work, 60 * HZ);

[Severity: Critical]
This is a pre-existing issue, but could this queue_delayed_work() call lead
to a use-after-free and timer corruption if it races with an executing
delete_work?

If a previous delete_work worker wakes up, clears the WORK_STRUCT_PENDING bit,
and blocks on kmod_dup_mutex:

Worker Thread:
  (clears WORK_STRUCT_PENDING)
  mutex_lock(&kmod_dup_mutex); // blocked

Concurrent module load:
kmod_dup_request_announce()
  mutex_lock(&kmod_dup_mutex);
  ...
  queue_delayed_work(..., &kmod_req->delete_work, 60 * HZ);
  mutex_unlock(&kmod_dup_mutex);

Because the pending bit was cleared by the worker, queue_delayed_work() will
successfully re-arm the timer.

When the worker thread acquires the mutex, it will unlink kmod_req and
unconditionally call kfree().  When the re-armed timer expires 60 seconds
later, the kernel will dereference the timer_list from freed memory, resulting
in a panic.

>  
>  out:
>  	mutex_unlock(&kmod_dup_mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1