linux-kernel - Re: [PATCH 1/1] tracing: Fix memory leak when reading set

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] [day] [month] [year] [list]

Message-ID: <20250219110513.12411861@gandalf.local.home>
Date: Wed, 19 Feb 2025 11:05:13 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Adrian Huang <adrianhuang0701@...il.com>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, Adrian Huang <ahuang12@...ovo.com>
Subject: Re: [PATCH 1/1] tracing: Fix memory leak when reading set_event
 file

On Wed, 19 Feb 2025 18:42:30 +0800
Adrian Huang <adrianhuang0701@...il.com> wrote:


> The root cause is that s_next() returns NULL when nothing is found.
> This results in s_stop() attempting to free a NULL pointer because its
> parameter p is NULL.
> 
> Fix the issue by freeing the memory appropriately when s_next() fails
> to find anything.
> 
> Fixes: b355247df104 ("tracing: Cache ":mod:" events for modules not loaded yet")
> Signed-off-by: Adrian Huang <ahuang12@...ovo.com>
> ---
>  kernel/trace/trace_events.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 4cb275316e51..c76353ad0a4e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1591,6 +1591,7 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
>  		return iter;
>  #endif
>  
> +	kfree(iter);
>  	return NULL;
>  }
>  

This most definitely needs a comment, as it will look like a bug otherwise.
Please add:

	/*
	 * The iter is allocated in s_start() and passed via the 'v'
	 * parameter. To stop the iterator, NULL must be returned. But
	 * the return value is what the 'v' parameter in s_stop() receives
	 * and frees. Free iter here as it will no longer be used.
	 */
	kfree(iter);

I would also change the variable 'p' in s_stop() to 'v' to be consistent.

Thanks,

-- Steve