linux-kernel - Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable

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

Message-ID: <20210401182810.GO4758@sirena.org.uk>
Date:   Thu, 1 Apr 2021 19:28:11 +0100
From:   Mark Brown <broonie@...nel.org>
To:     "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
Cc:     mark.rutland@....com, jpoimboe@...hat.com, jthierry@...hat.com,
        catalin.marinas@....com, will@...nel.org,
        linux-arm-kernel@...ts.infradead.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the
 stack trace unreliable

On Thu, Apr 01, 2021 at 12:43:25PM -0500, Madhavan T. Venkataraman wrote:

> >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> >> +	{ (unsigned long) &ftrace_graph_call, 0 },
> >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >> +	{ (unsigned long) ftrace_graph_caller, 0 },

> > It's weird that we take the address of ftrace_graph_call but not the
> > other functions - we should be consistent or explain why.  It'd probably
> > also look nicer to not nest the ifdefs, the dependencies in Kconfig will
> > ensure we only get things when we should.

> I have explained it in the comment in the FTRACE trampoline right above
> ftrace_graph_call().

Ah, right - it's a result of it being an inner label.  I'd suggest
putting a brief note right at that line of code explaining this (eg,
"Inner label, not a function"), it wasn't confusing due to the use of
that symbol but rather due to it being different from everything else
in the list and that's kind of lost in the main comment.

> So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address
> this as well as your comment by defining another label whose name is more meaningful
> to our use:

> +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
>         nop                             // If enabled, this will be replaced
>                                         // "b ftrace_graph_caller"
> #endif

I'm not sure we need to bother with that, you'd still need the & I think.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)