Re: [PATCH] module: dups: use strscpy() to copy module name in dup request

[sashiko-bot@kernel.org]

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 <naveen.osdev@gmail.com>

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