Re: [PATCH 1/5] module/dups: Fix use-after-free in kmod_dup_req lifetime handling

[Petr Pavlu <petr.pavlu@suse.com>]

On 7/20/26 2:32 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] A pre-existing use-after-free exists in `kmod_dup_request_complete()` due to `kmod_dup_request_announce()` queuing `complete_work` without holding a reference.
> --
> 
> --- Patch [1]: [PATCH 1/5] module/dups: Fix use-after-free in kmod_dup_req lifetime handling ---
> commit c16274209ebc300f334c8ae6c8a5bec3494aa562
> Author: Petr Pavlu <petr.pavlu@suse.com>
> 
> module/dups: Fix use-after-free in kmod_dup_req lifetime handling
> 
> The kmod dups code uses RCU to ensure that a kmod_dup_req instance is freed
> only after it is no longer referenced. When releasing an instance, the
> kmod_dup_request_delete() function removes the kmod_dup_req from the
> dup_kmod_reqs list, waits via synchronize_rcu() and finally frees it.
> However, this doesn't work correctly because parallel users referencing the
> instance in kmod_dup_request_exists_wait() don't enter an RCU read-side
> critical section. This can result in a use-after-free.
> 
> Fix the issue by changing the lifecycle management of kmod_dup_req to use
> reference counting.
> 
>> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
>> index 1d720a5311ba0..e1828e865edde 100644
>> --- a/kernel/module/dups.c
>> +++ b/kernel/module/dups.c
> 
> [ ... ]
> 
>> @@ -224,6 +241,10 @@ void kmod_dup_request_announce(char *module_name, int ret)
>>  
>>  	mutex_lock(&kmod_dup_mutex);
>>  
>> +	/*
>> +	 * Find the entry previously added in kmod_dup_request_exists_wait()
>> +	 * that is owned by the current task.
>> +	 */
>>  	kmod_req = kmod_dup_request_lookup(module_name);
>>  	if (!kmod_req)
>>  		goto out;
> 
> [Severity: Critical]
> This is a pre-existing issue, but is there still a use-after-free risk for
> complete_work even with the refcounting switch?
> 
> Looking further down in kmod_dup_request_announce(), it queues the
> asynchronous work item without incrementing the object's reference count:
> 
> 	queue_work(system_dfl_wq, &kmod_req->complete_work);
> 
> Because no reference is held for the work item, kmod_req could be freed by
> delete_work before kmod_dup_request_complete() finishes executing.
> 
> When the work executes in kmod_dup_request_complete() and dereferences
> kmod_req to call complete_all(&kmod_req->first_req_done), this could access
> freed memory.
> 
> Can concurrent auto-loads trigger this? For instance, if multiple tasks
> execute __request_module() concurrently:
> 
> Task 1 (wait=false)                 Task 2 (wait=true)
> kmod_dup_request_announce()
>   queue_work(complete_work)
>                                     kmod_dup_request_announce()
>                                       queue_work(complete_work)
> 
> Could the second complete_work execution be queued just before the 60-second
> timer expires, and run right after delete_work frees the object?
> 

I agree. The code has an issue when two concurrent request_module() are
active, one using wait=true and the other wait=false. When the
wait=false call wins and announces the result via
kmod_dup_request_announce(), it can complete the kmod_dup_req object
allocated by the wait=true call, which can later result in
a use-after-free.

Sashiko also reported the same issue in a reply to patch #2.

-- Petr