linux-kernel - Re: [PATCH v4 2/4] ftrace: Add support for function argument to graph tracer

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: <20250813195317.508a29aa@batman.local.home>
Date: Wed, 13 Aug 2025 19:53:17 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Sasha Levin <sashal@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 bpf@...r.kernel.org, Masami Hiramatsu <mhiramat@...nel.org>, Mark Rutland
 <mark.rutland@....com>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Sven Schnelle
 <svens@...ux.ibm.com>, Paul Walmsley <paul.walmsley@...ive.com>, Palmer
 Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Guo Ren
 <guoren@...nel.org>, Donglin Peng <dolinux.peng@...il.com>, Zheng Yejian
 <zhengyejian@...weicloud.com>
Subject: Re: [PATCH v4 2/4] ftrace: Add support for function argument to
 graph tracer

On Fri, 8 Aug 2025 22:24:05 -0400
Sasha Levin <sashal@...nel.org> wrote:

> So we've added a dynamically sized array to the end of
> ftrace_graph_ent_entry, but in struct fgraph_data, the saved entry is
> defined as:
> 
>    struct fgraph_data {
>        ...
>        union {
>            struct ftrace_graph_ent_entry ent;
>            struct fgraph_retaddr_ent_entry rent;
>        } ent;
>        ...
>    }
> 
> Which doesn't seem to have room for args?

No it doesn't :-p

> 
> The code in get_return_for_leaf() does:
> 
>    data->ent.ent = *curr;
> 
> This copies the struct, but curr points to a larger entry with args
> data. The copy operation only copies sizeof(struct
> ftrace_graph_ent_entry) bytes, which doesn't include the dynamic args
> array.
> 
> And then later functions (like print_graph_entry()) would go ahead and
> assume that iter->ent_size is sane and make a mess out of everything.
> 
> I can't test right now whether this actually fixes the issues or not,
> but I wanted to bring this up as this looks somewhat odd and I'm not too
> familiar with this code.

Thanks for the detail analysis, can you test this patch?

-- Steve

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 66e1a527cf1a..25ea71edb8da 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -35,6 +35,11 @@ struct fgraph_data {
 		struct ftrace_graph_ent_entry	ent;
 		struct fgraph_retaddr_ent_entry	rent;
 	} ent;
+	/*
+	 * The @args must be right after @ent, as it is where they
+	 * are stored in case the function graph tracer has arguments.
+	 */
+	unsigned long			args[FTRACE_REGS_MAX_ARGS];
 	struct ftrace_graph_ret_entry	ret;
 	int				failed;
 	int				cpu;
@@ -623,14 +628,29 @@ get_return_for_leaf(struct trace_iterator *iter,
 		next = ring_buffer_event_data(event);
 
 		if (data) {
+			int args_size;
+			int size;
+
 			/*
 			 * Save current and next entries for later reference
 			 * if the output fails.
 			 */
-			if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT))
+			if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
 				data->ent.rent = *(struct fgraph_retaddr_ent_entry *)curr;
-			else
+				size = offsetof(struct fgraph_retaddr_ent_entry, args);
+			} else {
 				data->ent.ent = *curr;
+				size = offsetof(struct ftrace_graph_ent_entry, args);
+			}
+
+			/* If this has args, then append them to after the ent. */
+			args_size = iter->ent_size - size;
+			if (args_size > sizeof(long) * FTRACE_REGS_MAX_ARGS)
+				args_size = sizeof(long) * FTRACE_REGS_MAX_ARGS;
+
+			if (args_size >= sizeof(long))
+				memcpy((void *)&data->ent.ent + size,
+				       (void*)curr + size, args_size);
 			/*
 			 * If the next event is not a return type, then
 			 * we only care about what type it is. Otherwise we can