On 7/20/26 2:32 PM, [email protected] 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 <[email protected]> > > 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
