linux-kernel - x86/kprobes bug? (was: [PATCH 1/3] x86/alternatives: Teach text_poke

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: <20191009130754.GL2311@hirez.programming.kicks-ass.net>
Date:   Wed, 9 Oct 2019 15:07:54 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Nadav Amit <nadav.amit@...il.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Song Liu <songliubraving@...com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        paulmck@...nel.org
Subject: x86/kprobes bug? (was: [PATCH 1/3] x86/alternatives: Teach
 text_poke_bp() to emulate instructions)

On Fri, Oct 04, 2019 at 10:45:40PM +0900, Masami Hiramatsu wrote:

> > > > >  	text_poke_bp(op->kp.addr, insn_buff, RELATIVEJUMP_SIZE,
> > > > > -		     op->optinsn.insn);
> > > > > +		     emulate_buff);
> > > > >  }
> > > 
> > > As argued in a previous thread, text_poke_bp() is broken when it changes
> > > more than a single instruction at a time.
> > > 
> > > Now, ISTR optimized kprobes does something like:
> > > 
> > > 	poke INT3
> > 
> > Hmm, it does this using text_poke(), but lacks a
> > on_each_cpu(do_sync_core, NULL, 1), which I suppose is OK-ish IFF you do
> > that synchronize_rcu_tasks() after it, but less so if you don't.
> > 
> > That is, without either, you can't really tell if the kprobe is in
> > effect or not.
> 
> Yes, it doesn't wait the change by design at this moment.

Right, this might surprise some, I suppose, and I might've found a small
issue with it, see below.

> > > 	synchronize_rcu_tasks() /* waits for all tasks to schedule
> > > 				   guarantees instructions after INT3
> > > 				   are unused */
> > > 	install optimized probe /* overwrites multiple instrctions with
> > > 				   JMP.d32 */
> > > 
> > > And the above then undoes that by:
> > > 
> > > 	poke INT3 on top of the optimzed probe
> > > 
> > > 	poke tail instructions back /* guaranteed safe because the
> > > 				       above INT3 poke ensures the
> > > 				       JMP.d32 instruction is unused */
> > > 
> > > 	poke head byte back
> 
> Yes, anyway, the last poke should recover another INT3... (for kprobe)

It does indeed.

> > > Is this correct? If so, we should probably put a comment in there
> > > explaining how all this is unusual but safe.

So from what I can tell of kernel/kprobes.c, what it does is something like:

ARM: (__arm_kprobe)
	text_poke(INT3)
	/* guarantees nothing, INT3 will become visible at some point, maybe */

     (kprobe_optimizer)
	if (opt) {
		/* guarantees the bytes after INT3 are unused */
		syncrhonize_rcu_tasks();
		text_poke_bp(JMP32);
		/* implies IPI-sync, kprobe really is enabled */
	}


DISARM: (__unregister_kprobe_top)
	if (opt) {
		text_poke_bp(INT3 + tail);
		/* implies IPI-sync, so tail is guaranteed visible */
	}
	text_poke(old);


FREE: (__unregister_kprobe_bottom)
	/* guarantees 'old' is visible and the kprobe really is unused, maybe */
	synchronize_rcu();
	free();


Now the problem is that I don't think the synchronize_rcu() at free
implies enough to guarantee 'old' really is visible on all CPUs.
Similarly, I don't think synchronize_rcu_tasks() is sufficient on the
ARM side either. It only provides the guarantee -provided- the INT3 is
actually visible. If it is not, all bets are off.

I'd feel much better if we switch arch_arm_kprobe() over to using
text_poke_bp(). Or at the very least add the on_each_cpu(do_sync_core)
to it.

Hmm?