linux-kernel - Re: [PATCH] fgraph: Copy args in intermediate storage with entry

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: <aKcYO5xvRv3M0Ngf@laps>
Date: Thu, 21 Aug 2025 08:59:39 -0400
From: Sasha Levin <sashal@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] fgraph: Copy args in intermediate storage with entry

On Wed, Aug 20, 2025 at 07:55:22PM -0400, Steven Rostedt wrote:
>From: Steven Rostedt <rostedt@...dmis.org>
>
>The output of the function graph tracer has two ways to display its
>entries. One way for leaf functions with no events recorded within them,
>and the other is for functions with events recorded inside it. As function
>graph has an entry and exit event, to simplify the output of leaf
>functions it combines the two, where as non leaf functions are separate:
>
> 2)               |              invoke_rcu_core() {
> 2)               |                raise_softirq() {
> 2)   0.391 us    |                  __raise_softirq_irqoff();
> 2)   1.191 us    |                }
> 2)   2.086 us    |              }
>
>The __raise_softirq_irqoff() function above is really two events that were
>merged into one. Otherwise it would have looked like:
>
> 2)               |              invoke_rcu_core() {
> 2)               |                raise_softirq() {
> 2)               |                  __raise_softirq_irqoff() {
> 2)   0.391 us    |                  }
> 2)   1.191 us    |                }
> 2)   2.086 us    |              }
>
>In order to do this merge, the reading of the trace output file needs to
>look at the next event before printing. But since the pointer to the event
>is on the ring buffer, it needs to save the entry event before it looks at
>the next event as the next event goes out of focus as soon as a new event
>is read from the ring buffer. After it reads the next event, it will print
>the entry event with either the '{' (non leaf) or ';' and timestamps (leaf).
>
>The iterator used to read the trace file has storage for this event. The
>problem happens when the function graph tracer has arguments attached to
>the entry event as the entry now has a variable length "args" field. This
>field only gets set when funcargs option is used. But the args are not
>recorded in this temp data and garbage could be printed. The entry field
>is copied via:
>
>  data->ent = *curr;
>
>Where "curr" is the entry field. But this method only saves the non
>variable length fields from the structure.
>
>Add a helper structure to the iterator data that adds the max args size to
>the data storage in the iterator. Then simply copy the entire entry into
>this storage (with size protection).
>
>Reported-by: Sasha Levin <sashal@...nel.org>
>Closes: https://lore.kernel.org/all/aJaxRVKverIjF4a6@lappy/
>Fixes: ff5c9c576e75 ("ftrace: Add support for function argument to graph tracer")
>Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

	Tested-by: Sasha Levin <sashal@...nel.org>

Thanks for the fix!

-- 
Thanks,
Sasha