Re: [PATCH v2 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK

From: Peter Zijlstra <peterz@infradead.org>
To: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@kernel.org>,
	Suleiman Souhlal <suleiman@google.com>, bpf <bpf@vger.kernel.org>,
	linux-kernel@vger.kernel.org, Borislav Petkov <bp@suse.de>,
	x86@kernel.org
Subject: Re: [PATCH v2 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK
Date: Thu, 8 Sep 2022 11:30:41 +0200	[thread overview]
Message-ID: <Yxm2QU1NJIkIyrrU@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20220908050855.w77mimzznrlp6pwe@treble>

On Wed, Sep 07, 2022 at 10:08:55PM -0700, Josh Poimboeuf wrote:
> On Thu, Sep 08, 2022 at 10:34:43AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for stopping
> > speculative execution after RET instruction, kprobes always failes to
> > check the probed instruction boundary by decoding the function body if
> > the probed address is after such sequence. (Note that some conditional
> > code blocks will be placed after function return, if compiler decides
> > it is not on the hot path.)
> > 
> > This is because kprobes expects someone (e.g. kgdb) puts the INT3 as
> > a software breakpoint and it will replace the original instruction.
> > But these INT3 are not such purpose, it doesn't need to recover the
> > original instruction.
> > 
> > To avoid this issue, memorize the branch target address during decoding
> > and if there is INT3, restart decoding from unchecked target address.
> 
> Hm, is kprobes conflicting with kgdb actually a realistic concern?
> Seems like a dangerous combination

Yeah, not in my book. If you use kgdb you'd better be ready to hold
pieces anyway, that thing is terrible.

> Wouldn't it be much simpler to just encode the knowledge that
> 
>   	if (CONFIG_RETHUNK && !X86_FEATURE_RETHUNK)
> 		// all rets are followed by four INT3s
> 	else if (CONFIG_SLS)
> 		// all rets are followed by one INT3

At the very least that doesn't deal with the INT3 after JMP thing the
compilers should do once they catch up. This issue seems to keep getting
lost, but is now part of the AMD Branch Type Confusion (it was disclosed
in an earlier thing, but I keep forgetting what that was called).

Once that lands the rules are:

 0-5 INT3 after RET, !CONFIG_RETHUNK && !CONFIG_SLS: 0
                     CONFIG_SLS: 1
		     CONFIG_RETHUNK: 4-5 depending on compiler version

 0-1 INT3 after RET: !CONFIG_SLS: 0
		     CONFIG_SLS: 0-1 depending on compiler version

Now, given we know the compiler version at build time, this could be
determined and used in kprobes, but meh.

We also *should* put an INT3 after indirect jumps when patching the
retpolines. We don't appear to do so, but that's recommended even for
Intel I think.

Let me go do a patch.