linux-kernel - Re: BPF tracing trampoline synchronization between update/freeing and execution?

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: <20200107090319.ggggnpkqfqdmldfy@pathway.suse.cz>
Date:   Tue, 7 Jan 2020 10:03:19 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Jann Horn <jannh@...gle.com>, bpf@...r.kernel.org,
        live-patching@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Steven Rostedt <rostedt@...dmis.org>,
        KP Singh <kpsingh@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        kernel list <linux-kernel@...r.kernel.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: BPF tracing trampoline synchronization between update/freeing
 and execution?

On Tue 2020-01-07 09:28:42, Petr Mladek wrote:
> On Mon 2020-01-06 17:56:54, Peter Zijlstra wrote:
> > On Mon, Jan 06, 2020 at 05:39:30PM +0100, Jann Horn wrote:
> > > Hi!
> > > 
> > > I was chatting with kpsingh about BPF trampolines, and I noticed that
> > > it looks like BPF trampolines (as of current bpf-next/master) seem to
> > > be missing synchronization between trampoline code updates and
> > > trampoline execution. Or maybe I'm missing something?
> > > 
> > > If I understand correctly, trampolines are executed directly from the
> > > fentry placeholders at the start of arbitrary kernel functions, so
> > > they can run without any locks held. So for example, if task A starts
> > > executing a trampoline on entry to sys_open(), then gets preempted in
> > > the middle of the trampoline, and then task B quickly calls
> > > BPF_RAW_TRACEPOINT_OPEN twice, and then task A continues execution,
> > > task A will end up executing the middle of newly-written machine code,
> > > which can probably end up crashing the kernel somehow?
> > > 
> > > I think that at least to synchronize trampoline text freeing with
> > > concurrent trampoline execution, it is necessary to do something
> > > similar to what the livepatching code does with klp_check_stack(), and
> > > then either use a callback from the scheduler to periodically re-check
> > > tasks that were in the trampoline or let the trampoline tail-call into
> > > a cleanup helper that is part of normal kernel text. And you'd
> > > probably have to gate BPF trampolines on
> > > CONFIG_HAVE_RELIABLE_STACKTRACE.
> > 
> > ftrace uses synchronize_rcu_tasks() to flip between trampolines iirc.
> 
> ftrace calls also schedule_on_each_cpu(ftrace_sync) to handle
> situations where RCU is not watching, see rcu_is_watching().
> 
> The following is called in ftrace_shutdown():
> 
> 	schedule_on_each_cpu(ftrace_sync);
> 
> 	if (IS_ENABLED(CONFIG_PREEMPTION))
> 		synchronize_rcu_tasks();
> 
> 	arch_ftrace_trampoline_free(ops);

Just to be sure. IMHO, the above should be enough to decide when
a ftrace-like trampoline could be freed. But it is not enough
to decide whether any task is still in the middle of the BPF
program that the trampoline jumped to.

You will likely need something more complicated to decide when it is safe
to free the BPF program itself. The stack check probably won't help.
I guess that the stack will be marked as unsafe in BPF program because
there will be no data for ORC unwinder. But some simple reference
counting might do the job.

Best Regards,
Petr