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

[Petr Pavlu <petr.pavlu@suse.com>]

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