[PATCH 0/2] module: Fix memory deallocation on error path in move_module()

[PATCH 0/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

The first patch is an actual fix. The second patch is a minor related
cleanup.

Petr Pavlu (2):
  module: Fix memory deallocation on error path in move_module()
  module: Avoid unnecessary return value initialization in move_module()

 kernel/module/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)


base-commit: bdc7f8c5adad50dad2ec762e317f8b212f5782ac
-- 
2.49.0

[PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

The function move_module() uses the variable t to track how many memory
types it has allocated and consequently how many should be freed if an
error occurs.

The variable is initially set to 0 and is updated when a call to
module_memory_alloc() fails. However, move_module() can fail for other
reasons as well, in which case t remains set to 0 and no memory is freed.

Fix the problem by setting t to MOD_MEM_NUM_TYPES after all memory types
have been allocated. Additionally, make the deallocation loop more robust
by not relying on the mod_mem_type_t enum having a signed integer as its
underlying type.

Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 08b59c37735e..322b38c0a782 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2614,7 +2614,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 static int move_module(struct module *mod, struct load_info *info)
 {
 	int i;
-	enum mod_mem_type t = 0;
+	enum mod_mem_type t;
 	int ret = -ENOMEM;
 	bool codetag_section_found = false;
 
@@ -2630,6 +2630,7 @@ static int move_module(struct module *mod, struct load_info *info)
 			goto out_err;
 		}
 	}
+	t = MOD_MEM_NUM_TYPES;
 
 	/* Transfer each section which specifies SHF_ALLOC */
 	pr_debug("Final section addresses for %s:\n", mod->name);
@@ -2693,8 +2694,8 @@ static int move_module(struct module *mod, struct load_info *info)
 	return 0;
 out_err:
 	module_memory_restore_rox(mod);
-	for (t--; t >= 0; t--)
-		module_memory_free(mod, t);
+	for (; t > 0; t--)
+		module_memory_free(mod, t - 1);
 	if (codetag_section_found)
 		codetag_free_module_sections(mod);
 
-- 
2.49.0

[PATCH 2/2] module: Avoid unnecessary return value initialization in move_module()

[Petr Pavlu]

All error conditions in move_module() set the return value by updating the
ret variable. Therefore, it is not necessary to the initialize the variable
when declaring it.

Remove the unnecessary initialization.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 322b38c0a782..06511607075c 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2615,7 +2615,7 @@ static int move_module(struct module *mod, struct load_info *info)
 {
 	int i;
 	enum mod_mem_type t;
-	int ret = -ENOMEM;
+	int ret;
 	bool codetag_section_found = false;
 
 	for_each_mod_mem_type(type) {
-- 
2.49.0

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

On 6/7/25 6:16 PM, Petr Pavlu wrote:
> The function move_module() uses the variable t to track how many memory
> types it has allocated and consequently how many should be freed if an
> error occurs.
> 
> The variable is initially set to 0 and is updated when a call to
> module_memory_alloc() fails. However, move_module() can fail for other
> reasons as well, in which case t remains set to 0 and no memory is freed.
> 
> Fix the problem by setting t to MOD_MEM_NUM_TYPES after all memory types
> have been allocated. Additionally, make the deallocation loop more robust
> by not relying on the mod_mem_type_t enum having a signed integer as its
> underlying type.
> 
> Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 08b59c37735e..322b38c0a782 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> [...]
>  	pr_debug("Final section addresses for %s:\n", mod->name);
> @@ -2693,8 +2694,8 @@ static int move_module(struct module *mod, struct load_info *info)
>  	return 0;
>  out_err:
>  	module_memory_restore_rox(mod);
> -	for (t--; t >= 0; t--)
> -		module_memory_free(mod, t);
> +	for (; t > 0; t--)
> +		module_memory_free(mod, t - 1);
>  	if (codetag_section_found)
>  		codetag_free_module_sections(mod);
>  

This can actually be simply:

	while (t--)
		module_memory_free(mod, t);

-- Petr

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Sami Tolvanen]

On Sun, Jun 08, 2025 at 09:25:34AM +0200, Petr Pavlu wrote:
> On 6/7/25 6:16 PM, Petr Pavlu wrote:
> > The function move_module() uses the variable t to track how many memory
> > types it has allocated and consequently how many should be freed if an
> > error occurs.
> > 
> > The variable is initially set to 0 and is updated when a call to
> > module_memory_alloc() fails. However, move_module() can fail for other
> > reasons as well, in which case t remains set to 0 and no memory is freed.
> > 
> > Fix the problem by setting t to MOD_MEM_NUM_TYPES after all memory types
> > have been allocated. Additionally, make the deallocation loop more robust
> > by not relying on the mod_mem_type_t enum having a signed integer as its
> > underlying type.
> > 
> > Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> > ---
> >  kernel/module/main.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 08b59c37735e..322b38c0a782 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > [...]
> >  	pr_debug("Final section addresses for %s:\n", mod->name);
> > @@ -2693,8 +2694,8 @@ static int move_module(struct module *mod, struct load_info *info)
> >  	return 0;
> >  out_err:
> >  	module_memory_restore_rox(mod);
> > -	for (t--; t >= 0; t--)
> > -		module_memory_free(mod, t);
> > +	for (; t > 0; t--)
> > +		module_memory_free(mod, t - 1);
> >  	if (codetag_section_found)
> >  		codetag_free_module_sections(mod);
> >  
> 
> This can actually be simply:
> 
> 	while (t--)
> 		module_memory_free(mod, t);

Looks correct to me either way.

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 2/2] module: Avoid unnecessary return value initialization in move_module()

[Sami Tolvanen]

On Sat, Jun 07, 2025 at 06:16:28PM +0200, Petr Pavlu wrote:
> All error conditions in move_module() set the return value by updating the
> ret variable. Therefore, it is not necessary to the initialize the variable
> when declaring it.
> 
> Remove the unnecessary initialization.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 322b38c0a782..06511607075c 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2615,7 +2615,7 @@ static int move_module(struct module *mod, struct load_info *info)
>  {
>  	int i;
>  	enum mod_mem_type t;
> -	int ret = -ENOMEM;
> +	int ret;
>  	bool codetag_section_found = false;
>  
>  	for_each_mod_mem_type(type) {

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

On 07/06/2025 18.16, Petr Pavlu wrote:
> The function move_module() uses the variable t to track how many memory
> types it has allocated and consequently how many should be freed if an
> error occurs.
> 
> The variable is initially set to 0 and is updated when a call to
> module_memory_alloc() fails. However, move_module() can fail for other
> reasons as well, in which case t remains set to 0 and no memory is freed.

Do you have a way to reproduce the leak?

> 
> Fix the problem by setting t to MOD_MEM_NUM_TYPES after all memory types
> have been allocated. Additionally, make the deallocation loop more robust
> by not relying on the mod_mem_type_t enum having a signed integer as its
> underlying type.
> 
> Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 08b59c37735e..322b38c0a782 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2614,7 +2614,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  static int move_module(struct module *mod, struct load_info *info)
>  {
>  	int i;
> -	enum mod_mem_type t = 0;
> +	enum mod_mem_type t;
>  	int ret = -ENOMEM;
>  	bool codetag_section_found = false;
>  
> @@ -2630,6 +2630,7 @@ static int move_module(struct module *mod, struct load_info *info)
>  			goto out_err;
>  		}
>  	}
> +	t = MOD_MEM_NUM_TYPES;

Why forcing to this? I think we want to loop from the last type found, in case
move_module() fails after this point. Here's my suggestion:

diff --git a/kernel/module/main.c b/kernel/module/main.c
index ada44860a868..c66881d2fb62 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2697,7 +2697,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 static int move_module(struct module *mod, struct load_info *info)
 {
        int i;
-       enum mod_mem_type t;
+       enum mod_mem_type t = MOD_TEXT;
        int ret;
        bool codetag_section_found = false;

@@ -2708,12 +2708,10 @@ static int move_module(struct module *mod, struct load_info *info)
                }

                ret = module_memory_alloc(mod, type);
-               if (ret) {
-                       t = type;
+               t = type;
+               if (ret)
                        goto out_err;
-               }
        }
-       t = MOD_MEM_NUM_TYPES;

        /* Transfer each section which specifies SHF_ALLOC */
        pr_debug("Final section addresses for %s:\n", mod->name)

Re: [PATCH 2/2] module: Avoid unnecessary return value initialization in move_module()

[Daniel Gomez]

On 07/06/2025 18.16, Petr Pavlu wrote:
> All error conditions in move_module() set the return value by updating the
> ret variable. Therefore, it is not necessary to the initialize the variable
> when declaring it.
> 
> Remove the unnecessary initialization.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>

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

> ---
>  kernel/module/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 322b38c0a782..06511607075c 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2615,7 +2615,7 @@ static int move_module(struct module *mod, struct load_info *info)
>  {
>  	int i;
>  	enum mod_mem_type t;
> -	int ret = -ENOMEM;
> +	int ret;

Nit:

We can "merge" all integers in one line.

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

On 6/10/25 8:51 PM, Daniel Gomez wrote:
> On 07/06/2025 18.16, Petr Pavlu wrote:
>> The function move_module() uses the variable t to track how many memory
>> types it has allocated and consequently how many should be freed if an
>> error occurs.
>>
>> The variable is initially set to 0 and is updated when a call to
>> module_memory_alloc() fails. However, move_module() can fail for other
>> reasons as well, in which case t remains set to 0 and no memory is freed.
> 
> Do you have a way to reproduce the leak?

I was only able to test it by directly inserting errors in
move_module().

> 
>>
>> Fix the problem by setting t to MOD_MEM_NUM_TYPES after all memory types
>> have been allocated. Additionally, make the deallocation loop more robust
>> by not relying on the mod_mem_type_t enum having a signed integer as its
>> underlying type.
>>
>> Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>> ---
>>  kernel/module/main.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index 08b59c37735e..322b38c0a782 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -2614,7 +2614,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>>  static int move_module(struct module *mod, struct load_info *info)
>>  {
>>  	int i;
>> -	enum mod_mem_type t = 0;
>> +	enum mod_mem_type t;
>>  	int ret = -ENOMEM;
>>  	bool codetag_section_found = false;
>>  
>> @@ -2630,6 +2630,7 @@ static int move_module(struct module *mod, struct load_info *info)
>>  			goto out_err;
>>  		}
>>  	}
>> +	t = MOD_MEM_NUM_TYPES;
> 
> Why forcing to this? I think we want to loop from the last type found, in case
> move_module() fails after this point. Here's my suggestion:
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index ada44860a868..c66881d2fb62 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2697,7 +2697,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  static int move_module(struct module *mod, struct load_info *info)
>  {
>         int i;
> -       enum mod_mem_type t;
> +       enum mod_mem_type t = MOD_TEXT;
>         int ret;
>         bool codetag_section_found = false;
> 
> @@ -2708,12 +2708,10 @@ static int move_module(struct module *mod, struct load_info *info)
>                 }
> 
>                 ret = module_memory_alloc(mod, type);
> -               if (ret) {
> -                       t = type;
> +               t = type;
> +               if (ret)
>                         goto out_err;
> -               }
>         }
> -       t = MOD_MEM_NUM_TYPES;
> 
>         /* Transfer each section which specifies SHF_ALLOC */
>         pr_debug("Final section addresses for %s:\n", mod->name)

This seems to be off by one. For instance, if the loop reaches the last
valid type in mod_mem_type, MOD_INIT_RODATA, and successfully allocates
its memory, the variable t gets set to MOD_INIT_RODATA. Subsequently, if
an error occurs later in move_module() and control is transferred to
out_err, the deallocation starts from t-1, and therefore MOD_INIT_RODATA
doesn't get freed.

If we want to always start from the last type found, the code would need
to be:

		[...]
		ret = module_memory_alloc(mod, type);
		if (ret)
			goto out_err;
		t = type + 1;
	}

I can adjust it in this way if it is preferred.

-- 
Thanks,
Petr

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

> This seems to be off by one. For instance, if the loop reaches the last
> valid type in mod_mem_type, MOD_INIT_RODATA, and successfully allocates
> its memory, the variable t gets set to MOD_INIT_RODATA. Subsequently, if
> an error occurs later in move_module() and control is transferred to
> out_err, the deallocation starts from t-1, and therefore MOD_INIT_RODATA
> doesn't get freed.
> 
> If we want to always start from the last type found, the code would need
> to be:
> 
> 		[...]
> 		ret = module_memory_alloc(mod, type);
> 		if (ret)
> 			goto out_err;
> 		t = type + 1;
> 	}
> 
> I can adjust it in this way if it is preferred.
> 

My earlier suggestion was incorrect. We can simply initialize the memory
type t to MOD_MEM_NUM_TYPES since it's only used in the error path of
module_memory_alloc().

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

On 6/14/25 11:28 PM, Daniel Gomez wrote:
>> This seems to be off by one. For instance, if the loop reaches the last
>> valid type in mod_mem_type, MOD_INIT_RODATA, and successfully allocates
>> its memory, the variable t gets set to MOD_INIT_RODATA. Subsequently, if
>> an error occurs later in move_module() and control is transferred to
>> out_err, the deallocation starts from t-1, and therefore MOD_INIT_RODATA
>> doesn't get freed.
>>
>> If we want to always start from the last type found, the code would need
>> to be:
>>
>> 		[...]
>> 		ret = module_memory_alloc(mod, type);
>> 		if (ret)
>> 			goto out_err;
>> 		t = type + 1;
>> 	}
>>
>> I can adjust it in this way if it is preferred.
>>
> 
> My earlier suggestion was incorrect. We can simply initialize the memory
> type t to MOD_MEM_NUM_TYPES since it's only used in the error path of
> module_memory_alloc().

Do you mean the following, or something else:

static int move_module(struct module *mod, struct load_info *info)
{
	int i;
	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
	int ret;
	bool codetag_section_found = false;

	for_each_mod_mem_type(type) {
		if (!mod->mem[type].size) {
			mod->mem[type].base = NULL;
			continue;
		}

		ret = module_memory_alloc(mod, type);
		if (ret) {
			t = type;
			goto out_err;
		}
	}

	[...]
}

-- 
Thanks,
Petr

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

> Do you mean the following, or something else:
> 
> static int move_module(struct module *mod, struct load_info *info)
> {
> 	int i;
> 	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
> 	int ret;
> 	bool codetag_section_found = false;
> 
> 	for_each_mod_mem_type(type) {
> 		if (!mod->mem[type].size) {
> 			mod->mem[type].base = NULL;
> 			continue;
> 		}
> 
> 		ret = module_memory_alloc(mod, type);
> 		if (ret) {
> 			t = type;
> 			goto out_err;
> 		}
> 	}
> 
> 	[...]
> }
> 

Yes, that's it. From your patch, moving MOD_MEM_NUM_TYPE assigment to the
initialization and use the while() loop suggested later on.

One thing though, we are "releasing" the memory even if we have skipped the
allocation in the first place. So, I think it would make sense to release only
for the types we have actually allocated. What do you think?

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

On 6/17/25 11:47 AM, Daniel Gomez wrote:
>> Do you mean the following, or something else:
>>
>> static int move_module(struct module *mod, struct load_info *info)
>> {
>> 	int i;
>> 	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
>> 	int ret;
>> 	bool codetag_section_found = false;
>>
>> 	for_each_mod_mem_type(type) {
>> 		if (!mod->mem[type].size) {
>> 			mod->mem[type].base = NULL;
>> 			continue;
>> 		}
>>
>> 		ret = module_memory_alloc(mod, type);
>> 		if (ret) {
>> 			t = type;
>> 			goto out_err;
>> 		}
>> 	}
>>
>> 	[...]
>> }
>>
> 
> Yes, that's it. From your patch, moving MOD_MEM_NUM_TYPE assigment to the
> initialization and use the while() loop suggested later on.

Ok.

> 
> One thing though, we are "releasing" the memory even if we have skipped the
> allocation in the first place. So, I think it would make sense to release only
> for the types we have actually allocated. What do you think?

I noticed this too, specifically because move_module() is inconsistent
in this regard with free_mod_mem(). The latter function contains:

if (mod_mem->size)
	module_memory_free(mod, type);

However, my preference is actually to update free_mod_mem() and remove
the check. The function module_memory_free() should be a no-op if
mod->base is NULL, similarly to how calling free(NULL) is a no-op.

-- 
Thanks,
Petr

Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

>> One thing though, we are "releasing" the memory even if we have skipped the
>> allocation in the first place. So, I think it would make sense to release only
>> for the types we have actually allocated. What do you think?
> 
> I noticed this too, specifically because move_module() is inconsistent
> in this regard with free_mod_mem(). The latter function contains:
> 
> if (mod_mem->size)
> 	module_memory_free(mod, type);
> 
> However, my preference is actually to update free_mod_mem() and remove
> the check. The function module_memory_free() should be a no-op if
> mod->base is NULL, similarly to how calling free(NULL) is a no-op.
> 

Sound good to me. Perhaps a different patch type for cleanup/refactor. The fix
here would be back-ported to stable branches. So these are 2 different things.