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.pa...@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
