linux-kernel - Re: Have compiler remove __fentry locations from overwritten weak functions

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: <20241015080149.GP16066@noisy.programming.kicks-ass.net>
Date: Tue, 15 Oct 2024 10:01:49 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: "Jose E. Marchesi" <jemarch@....org>,
	Indu Bhagat <indu.bhagat@...cle.com>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Zheng Yejian <zhengyejian@...weicloud.com>,
	Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: Have compiler remove __fentry locations from overwritten weak
 functions

On Mon, Oct 14, 2024 at 09:08:41PM -0400, Steven Rostedt wrote:
> There's a long standing issue with having fentry in weak functions that
> were overwritten. This was first caught when a "notrace" function was
> showing up in the /sys/kernel/tracing/available_filter_functions list.
> 
>    https://lore.kernel.org/all/20220412094923.0abe90955e5db486b7bca279@kernel.org/
> 
> What was happening is that ftrace uses kallsyms (what is basically listed
> by nm) 

Ah, if only. NM lists the symbol table, and that has strictly more
information than kallsyms, notably it includes symbol length. Using
symbol length it is trivial to distinguish the case you're tripping
over.

> to map the location of __fentry to the previous symbol ahead of it.
> Then it says that function is the name of the function for that __fentry.
> But when you have weak functions, you can have this:
> 
> 
>   int notrace do_not_watch_me(void)
>   {
> 	return whatever;
>   }
> 
>   void __weak define_me_in_arch(void)
>   {
> 	return;
>   }
> 
> The define_me_in_arch() gets a __fentry, but because it is overwritten, it
> becomes attached to the end of do_not_watch_me(). When ftrace asks kallsyms
> for the symbol name of that __fentry location, it returns
> do_not_watch_me(), and ftrace will simply list that.

Which is a kallsym bug that does not exists in the ELF space.

There is a patch set that tries to fix this here:

  https://lore.kernel.org/all/20240723063258.2240610-1-zhengyejian@huaweicloud.com/T/#u

Instead of adding size to kallsyms it adds extra symbols to denote the
holes, which ends up being similar.

> But I was thinking, since gcc (and perhaps clang?) now has:
> 
>   -mrecord-mcount
> 
> that creates the __mcount_loc section that ftrace uses to find where the
> __fentry's are. Perhaps the compiler can simply remove any of those
> __fentry's that happen to exist in a weak function that was overridden.
> 
> Would this be something that the compiler could do?

Linker, this is link time. The linker would not only have to drop the
(weak) symbol from the symbol table (which is all it really does), but
it would now have to go and muck about with other sections.

It would have to go re-write the __mcount_loc section and drop entrie(s)
that were inside the dropped symbol. But then, what about other extra
sections? For instance, we can have (and still patch) alternatives in
the 'dead' weak text.

Where does this stop?

LTO can do the right thing and completely eliminate the weak function,
but anything short of that is a really big ask. Esp. since this is a
problem of our own making -- kallsyms is a flawed representation of the
symbol table.