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.0On 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)
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
> 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().
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
> 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?
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
>> 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.
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
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
© 2016 - 2025 Red Hat, Inc.