linux-kernel - Re: [PATCH v2 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 for Android: free password hash cracker in your pocket

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

Message-Id: <20250220132848.eafa42f94b25c6431d43a292@kernel.org>
Date: Thu, 20 Feb 2025 13:28:48 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Adrian Huang <adrianhuang0701@...il.com>
Cc: Steven Rostedt <rostedt@...dmis.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 v2 1/1] tracing: Fix memory leak when reading set_event
 file

On Thu, 20 Feb 2025 11:15:28 +0800
Adrian Huang <adrianhuang0701@...il.com> wrote:

> From: Adrian Huang <ahuang12@...ovo.com>
> 
> kmemleak reports the following memory leak after reading set_event file:
> 
>   # cat /sys/kernel/tracing/set_event
> 
>   # cat /sys/kernel/debug/kmemleak
>   unreferenced object 0xff110001234449e0 (size 16):
>   comm "cat", pid 13645, jiffies 4294981880
>   hex dump (first 16 bytes):
>     01 00 00 00 00 00 00 00 a8 71 e7 84 ff ff ff ff  .........q......
>   backtrace (crc c43abbc):
>     __kmalloc_cache_noprof+0x3ca/0x4b0
>     s_start+0x72/0x2d0
>     seq_read_iter+0x265/0x1080
>     seq_read+0x2c9/0x420
>     vfs_read+0x166/0xc30
>     ksys_read+0xf4/0x1d0
>     do_syscall_64+0x79/0x150
>     entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> The issue can be reproduced regardless of whether set_event is empty or
> not. Here is an example about the valid content of set_event.
> 
>   # cat /sys/kernel/tracing/set_event
>   sched:sched_process_fork
>   sched:sched_switch
>   sched:sched_wakeup
>   *:*:mod:trace_events_sample
> 
> 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 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>

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>

Thank you,

> ---
> Changelog v2:
>  - Per Steven's suggestion: Add a comment to describe why to free memory
>    in s_next().
>  - Per Steven's suggestion: Change the variable 'p' to 'v' in s_stop()
> 
> ---
>  kernel/trace/trace_events.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 4cb275316e51..513de9ceb80e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1591,6 +1591,13 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
>  		return iter;
>  #endif
>  
> +	/*
> +	 * 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);
>  	return NULL;
>  }
>  
> @@ -1667,9 +1674,9 @@ static int s_show(struct seq_file *m, void *v)
>  }
>  #endif
>  
> -static void s_stop(struct seq_file *m, void *p)
> +static void s_stop(struct seq_file *m, void *v)
>  {
> -	kfree(p);
> +	kfree(v);
>  	t_stop(m, NULL);
>  }
>  
> -- 
> 2.34.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>