linux-kernel - Re: [PATCH 1/2] module: Fix memory deallocation on error path in move

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ae967353-71fa-4438-a84b-8f7e2815f485@kernel.org>
Date: Tue, 10 Jun 2025 20:51:09 +0200
From: Daniel Gomez <da.gomez@...nel.org>
To: Petr Pavlu <petr.pavlu@...e.com>, Luis Chamberlain <mcgrof@...nel.org>,
 Sami Tolvanen <samitolvanen@...gle.com>, Daniel Gomez <da.gomez@...sung.com>
Cc: linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] module: Fix memory deallocation on error path in
 move_module()

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@...e.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)