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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 7 Sep 2022 22:08:55 -0700
Josh Poimboeuf <jpoimboe@kernel.org> 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

I'm actually not sure, I don't recommend it. But it is safe just having
fail-safe.

> 
> Either way, this feels overengineered.  Sort of like implementing
> objtool in the kernel.
> 
> And it's incomplete: for a switch statement jump table (or C goto jump
> table like in BPF), you can't detect the potential targets of the
> indirect branch.

In that case, it just fails to detect instruction boundary (and anyway
optprobe just stops optimization if it finds the indirect jump). So it
is still fail safe.

> 
> 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

Maybe we should just ask kgdb if it is using breakpoint on that
function, and if so, just reject kprobe on it. Then, all INT3
can be just skipped. That may be more realistic solution.

Thank you, 

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>