Re: [PATCH v2] module: use strscpy() to copy module names in stats and dup tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete fix: Truncating overlong module names breaks deduplication and causes list bloat/memory leak.

Pre-existing issues:
- [High] Race condition in duplicate module load tracking causing Use-After-Free.
--

commit 609988fdef836043f78467817ec8e3cba34b5daf
Author: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>

module: use strscpy() to copy module names in stats and dup tracking

Both try_add_failed_module() and kmod_dup_request_exists_wait() use
memcpy() with strlen() to copy module names into fixed-size buffers.
Neither performs a bounds check on the copy. This patch replaces
memcpy() with strscpy() to ensure the copy is bounded and NUL-terminated.

> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 1d720a5311ba0..8b6f8f177de4f 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);

[Severity: High]
This is a pre-existing issue, but looking at kmod_dup_request_exists_wait(),
is there a race condition that could lead to a use-after-free?

In kmod_dup_request_exists_wait(), the kmod_req pointer is retrieved while
holding kmod_dup_mutex, but the lock is dropped before waiting on the
completion:

kernel/module/dups.c:kmod_dup_request_exists_wait() {
    ...
    mutex_unlock(&kmod_dup_mutex);
    ...
    ret = wait_for_completion_state(&kmod_req->first_req_done, TASK_KILLABLE);
    if (ret) {
        *dup_ret = ret;
        return true;
    }
    ...
    *dup_ret = kmod_req->dup_ret;
    ...
}

There doesn't appear to be a reference count or RCU read-side critical section
protecting kmod_req during this wait.

The completion is signaled by kmod_dup_request_complete(), which then
schedules kmod_dup_request_delete() via delayed work after 60 seconds.

If a duplicate request arrives exactly 60 seconds later, it could drop the
mutex just before the delayed work executes and frees the request:

kernel/module/dups.c:kmod_dup_request_delete() {
    ...
    mutex_lock(&kmod_dup_mutex);
    list_del_rcu(&kmod_req->list);
    synchronize_rcu();
    mutex_unlock(&kmod_dup_mutex);
    kfree(kmod_req);
}

When the duplicate thread resumes and accesses kmod_req->dup_ret, would it
dereference the freed memory?

[ ... ]

> diff --git a/kernel/module/stats.c b/kernel/module/stats.c
> index 3a9672f93a8ef..78b59bc05387f 100644
> --- a/kernel/module/stats.c
> +++ b/kernel/module/stats.c
> @@ -253,7 +253,7 @@ int try_add_failed_module(const char *name, enum fail_dup_mod_reason reason)
>  	mod_fail = kzalloc_obj(*mod_fail);
>  	if (!mod_fail)
>  		return -ENOMEM;
> -	memcpy(mod_fail->name, name, strlen(name));
> +	strscpy(mod_fail->name, name);

[Severity: Medium]
Does truncating overlong module names here break deduplication?

If try_add_failed_module() is called with an overlong name (which can
originate from the .modinfo ELF section), strscpy() safely truncates the
string stored in mod_fail->name.

However, the deduplication lookup uses strcmp(), which compares the truncated
stored name against the untruncated original string:

kernel/module/stats.c:try_add_failed_module() {
    ...
    list_for_each_entry_rcu(mod_fail, &dup_failed_modules, list, lockdep_is_held(&module_mutex)) {
        if (!strcmp(mod_fail->name, name)) {
            atomic_long_inc(&mod_fail->count);
    ...
}

This comparison will always fail for overlong names.

Would every attempt to load a malformed module allocate a new mod_fail struct
and add it to dup_failed_modules, potentially allowing an attacker with
CAP_SYS_MODULE to trigger unbounded list bloat and a memory leak?

>  	__set_bit(reason, &mod_fail->dup_fail_mask);
>  	atomic_long_inc(&mod_fail->count);
>  	list_add_rcu(&mod_fail->list, &dup_failed_modules);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/slebbzn7p6c4ybokolvey2sgvrooit66zjewwggqac2755ujgp@ptanmtdclmw5?part=1