Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing reference counting on `kmod_req` leads to Use-After-Free (UAF). - [High] Holding `kmod_dup_mutex` during `synchronize_rcu()` introduces severe system-wide latency spikes. -- commit c71132da4445b6f78bb4bc00760a72ed998ebd13 Author: Naveen Kumar Chaudhary <[email protected]> module: dups: use strscpy() to copy module name in dup request This commit replaces a fragile memcpy() call with strscpy() when copying the module name into a duplicate request. This ensures bounds-checking and explicit NUL-termination for the fixed-size buffer. > diff --git a/kernel/module/dups.c b/kernel/module/dups.c > index 1d720a5311ba0..33bddfb573171 100644 > --- a/kernel/module/dups.c > +++ b/kernel/module/dups.c > @@ -129,7 +129,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool > wait, int *dup_ret) > if (!new_kmod_req) > return false; > > - memcpy(new_kmod_req->name, module_name, strlen(module_name)); > + strscpy(new_kmod_req->name, module_name, MODULE_NAME_LEN); > INIT_WORK(&new_kmod_req->complete_work, kmod_dup_request_complete); > INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete); [Severity: High] This is a pre-existing issue, but does kmod_dup_request_delete() cause severe system-wide latency spikes? In kernel/module/dups.c:kmod_dup_request_delete(), synchronize_rcu() is called while holding the global kmod_dup_mutex: mutex_lock(&kmod_dup_mutex); list_del_rcu(&kmod_req->list); synchronize_rcu(); mutex_unlock(&kmod_dup_mutex); Because kmod_dup_mutex is acquired by every request_module() call system-wide, this stalls all unrelated module autoloading attempts during the RCU grace period. Furthermore, the RCU synchronization appears functionally useless because the only reader, kmod_dup_request_lookup(), strictly requires kmod_dup_mutex to be held rather than relying on RCU: list_for_each_entry_rcu(kmod_req, &dup_kmod_reqs, list, lockdep_is_held(&kmod_dup_mutex)) { > init_completion(&new_kmod_req->first_req_done); [Severity: Critical] This is a pre-existing issue, but is there a use-after-free risk for kmod_req here? The kmod_req structure lacks a reference count, and its lifetime is managed by an arbitrary 60-second delayed work queued during completion: kernel/module/dups.c:kmod_dup_request_complete() { ... queue_delayed_work(system_dfl_wq, &kmod_req->delete_work, 60 * HZ); ... } If a duplicate request task sleeping in wait_for_completion_state() is delayed for more than 60 seconds (e.g., due to a VM hypervisor pause, cgroup freezer, or severe scheduler starvation), kmod_dup_request_delete() will execute and kfree() the structure: kernel/module/dups.c:kmod_dup_request_delete() { ... kfree(kmod_req); ... } When the delayed task eventually resumes in kmod_dup_request_exists_wait(), it attempts to read kmod_req->dup_ret or remove itself from the freed waitqueue: kernel/module/dups.c:kmod_dup_request_exists_wait() { ... ret = wait_for_completion_state(&kmod_req->first_req_done, TASK_KILLABLE); if (ret) { *dup_ret = ret; return true; } /* Now the duplicate request has the same exact return value ... */ *dup_ret = kmod_req->dup_ret; ... } Would it be safer to use proper reference counting for kmod_req? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/v546667lngoo6dxwb7gqzf7liya2opdfeevrtar7iipsbkipfc@uh5fuaofcrty?part=1
