netdev - Re: [PATCH v2] ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function

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: <Yo7q6dwphFexGuRA@gmail.com>
Date:   Thu, 26 May 2022 04:50:17 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>, x86@...nel.org
Subject: Re: [PATCH v2] ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding
 weak function


* Steven Rostedt <rostedt@...dmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
> 
> If an unused weak function was traced, it's call to fentry will still
> exist, which gets added into the __mcount_loc table. Ftrace will use
> kallsyms to retrieve the name for each location in __mcount_loc to display
> it in the available_filter_functions and used to enable functions via the
> name matching in set_ftrace_filter/notrace. Enabling these functions do
> nothing but enable an unused call to ftrace_caller. If a traced weak
> function is overridden, the symbol of the function would be used for it,
> which will either created duplicate names, or if the previous function was
> not traced, it would be incorrectly listed in available_filter_functions
> as a function that can be traced.
> 
> This became an issue with BPF[1] as there are tooling that enables the
> direct callers via ftrace but then checks to see if the functions were
> actually enabled. The case of one function that was marked notrace, but
> was followed by an unused weak function that was traced. The unused
> function's call to fentry was added to the __mcount_loc section, and
> kallsyms retrieved the untraced function's symbol as the weak function was
> overridden. Since the untraced function would not get traced, the BPF
> check would detect this and fail.
> 
> The real fix would be to fix kallsyms to not show address of weak
> functions as the function before it. But that would require adding code in
> the build to add function size to kallsyms so that it can know when the
> function ends instead of just using the start of the next known symbol.

Yeah, so I actually have a (prototype...) objtool based kallsyms 
implementation in the (way too large) fast-headers tree that is both faster 
& allows such details in principle:

  431bca135cf8 kbuild/linker: Gather all the __kallsyms sections into a single table
  6bc7af02e402 objtool/kallsyms: Copy the symbol name and offset to the new __kallsyms ELF section
  e1e85b2fab9e objtool/kallsyms: Increase section size dynamically
  3528d607641b objtool/kallsyms: Use zero entry size for section
  2555dd62348a objtool/kallsyms: Output variable length strings
  c114b71f8547 objtool/kallsyms: Add kallsyms_offsets[] table
  cfcfce6cb51f objtool/kallsyms: Change name to __kallsyms_strs
  134160bb2de1 objtool/kallsyms: Split out process_kallsyms_symbols()
  33347a4b46e0 objtool/kallsyms: Add relocations
  86626e9e6603 objtool/kallsyms: Skip discarded sections
  7dd9fef6fbb0 kallsyms/objtool: Print out the new symbol table on the kernel side
  c82d94b33a1f objtool/kallsyms: Use struct kallsyms_entry
  a66ee5034008 objtool/kallsyms, kallsyms/objtool: Share 'struct kallsyms_entry' definition
  e496159e5282 kallsyms/objtool: Process entries
  47fc63ef3fa8 objtool/kallsyms: Switch to 64-bit absolute relocations
  c54fcc03cd64 objtool/kallsyms: Fix initial offset
  eac3c107aa6e kallsyms/objtool: Emit System.map-alike symbol list
  ebfb7e14b8ca objtool/kallsyms: Skip undefined symbols
  25b69ef5666b objtool/kallsyms: Update symbol filter
  3b26a82c733f objtool/kallsyms: Add the CONFIG_KALLSYMS_FAST Kconfig option & its related Kconfig switches
  9350c25942f8 kallsyms/objtool: Make the kallsyms work even if the generic tables are not there
  4a0a120bde05 objtool/kallsyms, x86/relocs: Detect & ignore __kallsyms_offset section relocations
  87c5244f1fa8 kallsyms/objtool: Introduce linear table of symbol structures: kallsyms_syms[]
  51dafdefc61f kallsyms/objtool: Split fast vs. generic functions
  e4123a40125f kallsyms/objtool: Sort symbols by address and deduplicate them
  2d738c69965a kallsyms/objtool: Utilize the kallsyms_syms[] table in kallsyms_expand_symbol() and kallsyms_sym_address()
  997ffe217a34 kallsyms/objtool: Port kallsyms_relative_base functionality to the kallsyms_syms[] offsets

> In the mean time, this is a work around. Add a FTRACE_MCOUNT_MAX_OFFSET
> macro that if defined, ftrace will ignore any function that has its call
> to fentry/mcount that has an offset from the symbol that is greater than
> FTRACE_MCOUNT_MAX_OFFSET.
> 
> If CONFIG_HAVE_FENTRY is defined for x86, define FTRACE_MCOUNT_MAX_OFFSET
> to zero, which will have ftrace ignore all locations that are not at the
> start of the function.
> 
> [1] https://lore.kernel.org/all/20220412094923.0abe90955e5db486b7bca279@kernel.org/
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

LGTM.

I suppose you'd like to merge this via the tracing tree? If so:

  Acked-by: Ingo Molnar <mingo@...nel.org>

Thanks,

	Ingo