[PATCH v2] module: fix lost error code from codetag_load_module()

[PATCH v2] module: fix lost error code from codetag_load_module()

[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.

Fixes: 044d2aee6c57 ("alloc_tag: handle module codetag load errors as module load failures")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Bradley Morgan <brads@mainlining.org>
Cc: stable@vger.kernel.org
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
v2: also free mod->klp_info of livepatch modules on this
    error path (Petr Pavlu).

v1 link: https://lore.kernel.org/all/20260824014736.35026-1-hao.ge@linux.dev/
---
 kernel/module/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 46dd8d25a605..c32f1d370b73 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -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:
-- 
2.25.1

Re: [PATCH v2] module: fix lost error code from codetag_load_module()

[sashiko-bot]

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 <hao.ge@linux.dev>

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.

> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index d0e1e0bd2ad06..c1b34dc1e89ac 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -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?

>  
>  	/* 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:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827030503.49171-1-hao.ge@linux.dev?part=1

Re: [PATCH v2] module: fix lost error code from codetag_load_module()

[Daniel Gomez]

On 2026-08-27T11:05:03+08:00, Hao Ge <hao.ge@linux.dev> wrote:
> 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 <sashiko-bot@kernel.org>
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Reviewed-by: Bradley Morgan <brads@mainlining.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Ge <hao.ge@linux.dev>

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

With that,

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

Re: [PATCH v2] module: fix lost error code from codetag_load_module()

[Hao Ge]

Hi Daniel


On 2026/9/8 03:42, Daniel Gomez wrote:
> 
> 
> On 2026-08-27T11:05:03+08:00, Hao Ge <hao.ge@linux.dev> wrote:
>> 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 <sashiko-bot@kernel.org>
>> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> 
> 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
     
> With that,
> 
> Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
> 
> 

Re: [PATCH v2] module: fix lost error code from codetag_load_module()

[Daniel Gomez]

On 2026-09-08T17:50:33+08:00, Hao Ge <hao.ge@linux.dev> wrote:
> 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.

Re: [PATCH v2] module: fix lost error code from codetag_load_module()

[Hao Ge]

Hi Daniel


On 2026/9/10 06:00, Daniel Gomez wrote:
> On 2026-09-08T17:50:33+08:00, Hao Ge <hao.ge@linux.dev> wrote:
>> 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 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.

> 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

Re: [PATCH v2] module: fix lost error code from codetag_load_module()

[Hao Ge]

Hi Daniel

Thanks for you comment.

On 2026/9/10 06:00, Daniel Gomez wrote:
> On 2026-09-08T17:50:33+08:00, Hao Ge <hao.ge@linux.dev> wrote:
>> 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 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.

> 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