linux-kernel - Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace

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: <20241002081037.e9b825f7456ce4815eccad1b@kernel.org>
Date: Wed, 2 Oct 2024 08:10:37 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Will Deacon <will@...nel.org>, Catalin Marinas
 <catalin.marinas@....com>, linux-arm-kernel@...ts.infradead.org,
 "Masami Hiramatsu (Google)" <mhiramat@...nel.org>, Florent Revest
 <revest@...omium.org>, linux-trace-kernel@...r.kernel.org, LKML
 <linux-kernel@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, Jiri
 Olsa <jolsa@...nel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>, Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with
 ftrace_regs

On Mon, 30 Sep 2024 14:55:48 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Tue, 17 Sep 2024 10:55:39 +0100
> Will Deacon <will@...nel.org> wrote:
> 
> > The arm64 part looks good to me, although passing a partially-populated
> > struct out of asm feels like it's going to cause us hard-to-debug
> > problems down the line if any of those extra fields get used. How hard
> > would it be to poison the unpopulated members of 'ftrace_regs'?
> 
> The purpose of creating ftrace_regs was to allow a partially populated
> pt_regs to be sent around, as Thomas Gleixner and Peter Zijlstra were
> against using pt_regs that were not fully populated. Hence, I created
> "ftrace_regs" for this purpose.
> 
> ftrace_regs should never be accessed via its internal elements but only with
> its accessor functions, as depending on the arch or functionality used, the
> content of the structure should never be trusted. The accessor functions
> will do all the verification needed.
> 
> I may add some compiler hacks to enforce this. Something like:
> 
> struct ftrace_regs {
> 	void *nothing_to_see_here;
> };

Yeah, OK. But sizeof(fregs) may be changed. (Shouldn't we do too?)

> 
> And then change the arch code to be something like:
> 
> // in arch/arm64/include/asm/ftrace.h:
> 
> struct arch_ftrace_regs {
>         /* x0 - x8 */
>         unsigned long regs[9];
> 
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>         unsigned long direct_tramp;
> #else
>         unsigned long __unused;
> #endif
> 
>         unsigned long fp;
>         unsigned long lr;
> 
>         unsigned long sp;
>         unsigned long pc;
> };

And if it is pt_regs compatible, 

#define arch_ftrace_regs pt_regs

?

> 
> #define get_arch_ftrace_regs(fregs) ((struct arch_ftrace_regs *)(fregs))
> 
> static __always_inline void
> ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
>                                     unsigned long pc)
> {
> 	struct arch_ftrace_regs *afregs = get_ftrace_regs(fregs);
>         afregs->pc = pc;
> }
> 
> 
> -- Steve


Thanks,

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