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

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 [off-list ref]

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.

quoted

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

[ ... ]

quoted

@@ -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?