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

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

[Naveen Kumar Chaudhary]

kmod_dup_request_exists_wait() uses memcpy() with strlen(module_name) to
copy into new_kmod_req->name, a fixed-size char[MODULE_NAME_LEN] buffer.
This does not bounds-check the copy and does not explicitly NUL-terminate.
The buffer is zeroed from kzalloc_obj() so NUL-termination happens to
work, but the pattern is fragile and lacks an explicit bounds check.

Replace with strscpy() which bounds the copy and guarantees
NUL-termination.

Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
---
 kernel/module/dups.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 1d720a5311ba..33bddfb57317 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);
 	init_completion(&new_kmod_req->first_req_done);
-- 
2.43.0

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

[sashiko-bot]

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

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

[Petr Pavlu]

On 6/3/26 6:25 PM, Naveen Kumar Chaudhary wrote:
> kmod_dup_request_exists_wait() uses memcpy() with strlen(module_name) to
> copy into new_kmod_req->name, a fixed-size char[MODULE_NAME_LEN] buffer.
> This does not bounds-check the copy and does not explicitly NUL-terminate.
> The buffer is zeroed from kzalloc_obj() so NUL-termination happens to
> work, but the pattern is fragile and lacks an explicit bounds check.
> 
> Replace with strscpy() which bounds the copy and guarantees
> NUL-termination.
> 
> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
> ---
>  kernel/module/dups.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 1d720a5311ba..33bddfb57317 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);
>  	init_completion(&new_kmod_req->first_req_done);

This can be shortened to:

strscpy(new_kmod_req->name, module_name);

I also suggest merging this patch and the second patch [1], which cleans
up the same issue in the module stats code, into one.

[1] https://lore.kernel.org/linux-modules/jmm7r4r3k3qt767tl7lojglosgc3umhc63cdp2fckdkgb3fzki@3fgvxgvzo5ex/

-- 
Thanks,
Petr

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

[Naveen Kumar Chaudhary]

Both try_add_failed_module() and kmod_dup_request_exists_wait() use
memcpy() with strlen() to copy module names into fixed-size
char[MODULE_NAME_LEN] buffers. Neither performs a bounds check on the
copy. Current callers always pass names originating from
mod->name (itself char[MODULE_NAME_LEN]), so this is not exploitable
today. However both functions accept a plain const char * with no
documented length contract, making them latent buffer overflows if a
future caller passes a longer string.

Replace memcpy() with strscpy() in both sites, which bounds the copy
to MODULE_NAME_LEN and always NUL-terminates.

Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
---

v1 -> v2:
	- Dropped third argument to strscpy
	- Merged the other patch https://lore.kernel.org/linux-modules/jmm7r4r3k3qt767tl7lojglosgc3umhc63cdp2fckdkgb3fzki@3fgvxgvzo5ex/

Thanks Petr for the reviews. Taken care of the suggestions.

Regards,
Naveen

 kernel/module/dups.c  | 2 +-
 kernel/module/stats.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 1d720a5311ba..33bddfb57317 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);
 	INIT_WORK(&new_kmod_req->complete_work, kmod_dup_request_complete);
 	INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
 	init_completion(&new_kmod_req->first_req_done);
diff --git a/kernel/module/stats.c b/kernel/module/stats.c
index 3a9672f93a8e..08724baca773 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);
 	__set_bit(reason, &mod_fail->dup_fail_mask);
 	atomic_long_inc(&mod_fail->count);
 	list_add_rcu(&mod_fail->list, &dup_failed_modules);
-- 
2.43.0

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

[sashiko-bot]

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