Skip to contentSkip to conversation

module: fix lost error code from codetag_load_module()

PATCH Applied Hao Ge 3 participants 7 messages Aug 27 – Sep 10

Subsystems (2)
Stable branch Module support
lore
Share & tools
lore

Hao Ge

author
#0 lore raw

PATCH v2 1/1 module: fix lost error code from codetag_load_module()

If codetag_load_module() fails, err is not set to reflect the failure and load_module() returns 0 after the module has been torn down.

Also, if the module is a livepatch, mod->klp_info allocated by copy_module_elf() leaks on this error path. Free it via a new livepatch_cleanup label.

Fixes 044d2aee6c57 ("alloc_tag: handle module codetag load errors as module load failures")
Reported-by Sashiko <[email protected]>
Suggested-by Petr Pavlu <[email protected]>
Reviewed-by Bradley Morgan <[email protected]>
Cc [email protected]
Signed-off-by Hao Ge <[email protected]>

---
v2: also free mod->klp_info of livepatch modules on this
error path (Petr Pavlu).

v1 link: https://lore.kernel.org/all/[email protected]/

---
 kernel/module/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
kernel/module/main.c +6 -2
index 46dd8d25a605..c32f1d370b73 100644
@@ -3568,8 +3568,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
 			goto sysfs_cleanup;
 	}
 
-	if (codetag_load_module(mod))
-		goto sysfs_cleanup;
+	err = codetag_load_module(mod);
+	if (err)
+		goto livepatch_cleanup;
 
 	/* Get rid of temporary copy. */
 	free_copy(info, flags);

@@ -3579,6 +3580,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
+ livepatch_cleanup:
+	if (is_livepatch_module(mod))
+		free_module_elf(mod);
  sysfs_cleanup:
 	mod_sysfs_teardown(mod);
  coming_cleanup:
Signature · 1 line
2.25.1
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

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

Pre-existing issues:
- [High] Missing codetag_unload_module(mod) on codetag_load_module() failure permanently strands partially initialized codetags, causing a Use-After-Free when the freed module is subsequently accessed. --

commit 46e63afac7b776756489b1e2ad6cec3cf1e11513
Author: Hao Ge <[email protected]>

module: fix lost error code from codetag_load_module()

If codetag_load_module() fails, err is not set to reflect the failure and load_module() returns 0 after the module has been torn down.

kernel/module/main.c static int load_module(struct load_info *info, const char __user *uargs, · line 3586

Quoted 13 lines
kernel/module/main.c +3 -2
index d0e1e0bd2ad06..c1b34dc1e89ac 100644
@@ -3581,8 +3581,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
 			goto sysfs_cleanup;
 	}
 
-	if (codetag_load_module(mod))
-		goto sysfs_cleanup;
+	err = codetag_load_module(mod);
+	if (err)
+		goto livepatch_cleanup;

[Severity: High]
This isn't a bug introduced by this patch, but missing codetag_unload_module() on codetag_load_module() failure permanently strands partially initialized codetags, causing a use-after-free when the freed module is subsequently accessed.

If a module load is initiated, and codetag_load_module() iterates through registered codetag_types:

lib/codetag.c:codetag_load_module() {
    ...
		ret = codetag_module_init(cttype, mod);
		if (ret)
			break;
    ...
}

If an early iteration succeeds but a subsequent one fails (e.g., kmalloc failure inside codetag_module_init()), codetag_load_module() breaks and returns the error without unwinding previously allocated cmod entries.

load_module() receives the error, jumps to livepatch_cleanup and eventually calls module_deallocate(mod, info) to free the module memory. However, codetag_unload_module(mod) is never invoked on this error path, leaving cmod entries pointing to freed module memory inside the successfully initialized cttype->mod_idr structures.

When subsequent codetag operations (like reading /proc/allocinfo) iterate over cttype->mod_idr, they will access the freed module memory.

Should this error path invoke codetag_unload_module(mod) to clean up the partially registered state before tearing down and freeing the module memory?

Quoted 13 lines
kernel/module/main.c +3 -0
 	/* Get rid of temporary copy. */
 	free_copy(info, flags);

@@ -3592,6 +3593,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
+ livepatch_cleanup:
+	if (is_livepatch_module(mod))
+		free_module_elf(mod);
  sysfs_cleanup:
 	mod_sysfs_teardown(mod);
  coming_cleanup:
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer Reviewed-by on v2 1/1
#2 lore raw
In reply to Hao Ge
Quoted 7 lines · Hao Ge

If codetag_load_module() fails, err is not set to reflect the failure and load_module() returns 0 after the module has been torn down.

Also, if the module is a livepatch, mod->klp_info allocated by copy_module_elf() leaks on this error path. Free it via a new livepatch_cleanup label.

Quoted 6 lines
Fixes 044d2aee6c57 ("alloc_tag: handle module codetag load errors as module load failures")
Reported-by Sashiko <[email protected]>
Suggested-by Petr Pavlu <[email protected]>
Reviewed-by Bradley Morgan <[email protected]>
Cc [email protected]
Signed-off-by Hao Ge <[email protected]>

We need the unwind from codetag_unload_module() as reported by sashiko.

With that,

Reviewed-by Daniel Gomez <[email protected]>
author
#3 lore raw

Hi Daniel

Quoted 10 lines · Daniel Gomez
Quoted 7 lines · Hao Ge

If codetag_load_module() fails, err is not set to reflect the failure and load_module() returns 0 after the module has been torn down.

Also, if the module is a livepatch, mod->klp_info allocated by copy_module_elf() leaks on this error path. Free it via a new livepatch_cleanup label.

Quoted 9 lines
Quoted 6 lines
Fixes 044d2aee6c57 ("alloc_tag: handle module codetag load errors as module load failures")
Reported-by Sashiko <[email protected]>
Suggested-by Petr Pavlu <[email protected]>
Reviewed-by Bradley Morgan <[email protected]>
Cc [email protected]
Signed-off-by Hao Ge <[email protected]>

We need the unwind from codetag_unload_module() as reported by sashiko.

Thanks for you review.

I skipped it because alloc_tag is the only codetag type and it already cleans up after itself.

With a second codetag type the problem Sashiko reported could indeed be triggered.

My thinking at the time was to do it in codetag itself, as a separate patch, since it feels odd to unload something whose load just failed.

Happy to hear your thoughts on this.

Thanks
Best Regards
Hao

Quoted 5 lines

With that,

Reviewed-by Daniel Gomez <[email protected]>
maintainer
#4 lore raw
Quoted 10 lines · Hao Ge

On 2026/9/8 03:42, Daniel Gomez wrote:

I skipped it because alloc_tag is the only codetag type and it already cleans up after itself.

With a second codetag type the problem Sashiko reported could indeed be triggered.

My thinking at the time was to do it in codetag itself, as a separate patch, since it feels odd to unload something whose load just failed.

Then, why returning an error at all? I don't see why not doing it inside codetag itself wouldn't work but it's a "bigger" change. IMO, the error is supposed to be handled at load_module() then, the unwinding as the rest.

author
#5 lore raw

Hi Daniel

Quoted 6 lines · Daniel Gomez
Quoted 5 lines · Hao Ge

On 2026/9/8 03:42, Daniel Gomez wrote:

I skipped it because alloc_tag is the only codetag type and it already cleans up after itself.

Quoted 7 lines
Quoted 5 lines

With a second codetag type the problem Sashiko reported could indeed be triggered.

My thinking at the time was to do it in codetag itself, as a separate patch, since it feels odd to unload something whose load just failed.

Then, why returning an error at all? I don't see why not doing it inside

Codetag is a generic framework with a dedicated section in modules. We keep this error path to account for possible future data stored in this section, allowing us to perform security and validity checks for codetag within codetag_load_module.

Quoted 5 lines

codetag itself wouldn't work but it's a "bigger" change. IMO, the error is supposed to be handled at load_module() then, the unwinding as the rest.

I'm worried about this as well. Invoking codetag_unload_module () here brings kvfree_rcu_barrier () into the error path and triggers two runs of ->free_section_mem (), (used=true from the unload, used=false later from module_deallocate).

Perhaps we could put this patch on hold for now, until I come up with a proper solution? The intention is to let codetag take care of its own cleanup, preserving the logic in our existing patch.

How does this sound?

Thanks
Best Regards
Hao

author
#6 lore raw
In reply to Daniel Gomez

Hi Daniel

Thanks for you comment.

Quoted 6 lines · Daniel Gomez
Quoted 5 lines · Hao Ge

On 2026/9/8 03:42, Daniel Gomez wrote:

I skipped it because alloc_tag is the only codetag type and it already cleans up after itself.

Quoted 7 lines
Quoted 5 lines

With a second codetag type the problem Sashiko reported could indeed be triggered.

My thinking at the time was to do it in codetag itself, as a separate patch, since it feels odd to unload something whose load just failed.

Then, why returning an error at all? I don't see why not doing it inside

codetag is a generic framework with a dedicated section in modules. We keep this error path to account for possible future data stored in this section, allowing us to perform security and validity checks for codetag within codetag_load_module.

Quoted 5 lines

codetag itself wouldn't work but it's a "bigger" change. IMO, the error is supposed to be handled at load_module() then, the unwinding as the rest.

I'm worried about this as well. Invoking codetag_unload_module() here brings kvfree_rcu_barrier() into the error path and triggers two runs of ->free_section_mem() (used=true from the unload, used=false later from module_deallocate).

My idea is to let codetag handle its own cleanup while keeping the logic of our current patch. Maybe we can put this patch on hold temporarily until I work out a proper solution. How does this sound?

Thanks
Best Regards
Hao