Re: KRETPROBES are broken since kernel 5.8

[Adam Zabrocki <pi3@pi3.com.pl>]

Hi,

On Thu, Dec 10, 2020 at 10:25:07AM +0900, Masami Hiramatsu wrote:
> Hi Adam,
> 
> Thank you for reporting and debugging, actually we had investigated this
> issue in Aug. Please read this thread.
> 
> https://lkml.kernel.org/r/8816bdbbc55c4d2397e0b02aad2825d3@trendmicro.com
> 

Thanks for the link. I've read the entire history of proposed fix - very 
informative :)

> We finally fixed this issue by commit e03b4a084ea6 ("kprobes: Remove NMI
> context check") and commit 645f224e7ba2 ("kprobes: Tell lockdep about
> kprobe nesting"). Yeah, it will be in the v5.10.
> 
> Could you try to test whether these commits can solve the issue?

I've applied these commits on the top of kernel 5.9.7 and verified that it 
works well. Non-optimized KRETPROBES are back to life.

However, there might be another issue which I wanted to brought / discuss - 
problem with optimizer. Until kernel 5.9 KRETPROBE on e.g. 
'ftrace_enable_sysctl' function was correctly optimized without any problems. 
Since 5.9 it can't be optimized anynmore. I didn't see any changes in the 
sources regarding the optimizer, neither function itself.
When I looked at the generated vmlinux binary, I can see that GCC generated 
padding at the end of this function using INT3 opcode:

...
ffffffff8130528b:       41 bd f0 ff ff ff       mov    $0xfffffff0,%r13d
ffffffff81305291:       e9 fe fe ff ff          jmpq   ffffffff81305194 <ftrace_enable_sysctl+0x114>
ffffffff81305296:       cc                      int3
ffffffff81305297:       cc                      int3
ffffffff81305298:       cc                      int3
ffffffff81305299:       cc                      int3
ffffffff8130529a:       cc                      int3
ffffffff8130529b:       cc                      int3
ffffffff8130529c:       cc                      int3
ffffffff8130529d:       cc                      int3
ffffffff8130529e:       cc                      int3
ffffffff8130529f:       cc                      int3

Such padding didn't exist in this function in generated images for older 
kernels. Nevertheless, such padding is not unusual and it's pretty common.

Optimizer logic fails here:

try_to_optimize_kprobe() -> alloc_aggr_kprobe() -> __prepare_optimized_kprobe()
-> arch_prepare_optimized_kprobe() -> can_optimize():

    /* Decode instructions */
    addr = paddr - offset;
    while (addr < paddr - offset + size) { /* Decode until function end */
        unsigned long recovered_insn;
        if (search_exception_tables(addr))
            /*
             * Since some fixup code will jumps into this function,
             * we can't optimize kprobe in this function.
             */
            return 0;
        recovered_insn = recover_probed_instruction(buf, addr);
        if (!recovered_insn)
            return 0;
        kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
        insn_get_length(&insn);
        /* Another subsystem puts a breakpoint */
        if (insn.opcode.bytes[0] == INT3_INSN_OPCODE)
            return 0;
        /* Recover address */
        insn.kaddr = (void *)addr;
        insn.next_byte = (void *)(addr + insn.length);
        /* Check any instructions don't jump into target */
        if (insn_is_indirect_jump(&insn) ||
            insn_jump_into_range(&insn, paddr + INT3_INSN_SIZE,
                     DISP32_SIZE))
            return 0;
        addr += insn.length;
    }

One of the check tries to protect from the situation when another subsystem 
puts a breakpoint there as well:

        /* Another subsystem puts a breakpoint */
        if (insn.opcode.bytes[0] == INT3_INSN_OPCODE)
            return 0;

However, that's not the case here. INT3_INSN_OPCODE is placed at the end of 
the function as padding (and protect from NOP-padding problems).

I wonder, if optimizer should take this special case into account? Is it worth 
to still optimize such functions when they are padded with INT3?

> If it is OK, we should backport those to stable tree.

Agreed.

Thanks,
Adam

> Thank you,
> 
> On Wed, 9 Dec 2020 22:50:01 +0100
> Adam Zabrocki <pi3@pi3.com.pl> wrote:
> 
> > Hello,
> > 
> > Starting from kernel 5.8 all non-optimized kretprobes don't work. Until 5.8,
> > when #DB exception was raised, entry to the NMI was not fully performed. Among
> > others, the following logic was executed:
> > https://elixir.bootlin.com/linux/v5.7.19/source/arch/x86/kernel/traps.c#L589
> > 
> >     if (!user_mode(regs)) {
> >         rcu_nmi_enter();
> >         preempt_disable();
> >     }
> > 
> > In some older kernels function ist_enter() was called instead. Inside this
> > function we can see the following logic:
> > https://elixir.bootlin.com/linux/v5.7.19/source/arch/x86/kernel/traps.c#L91
> > 
> >     if (user_mode(regs)) {
> >         RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> >     } else {
> >         /*
> >          * We might have interrupted pretty much anything.  In
> >          * fact, if we're a machine check, we can even interrupt
> >          * NMI processing.  We don't want in_nmi() to return true,
> >          * but we need to notify RCU.
> >          */
> >         rcu_nmi_enter();
> >     }
> > 
> >     preempt_disable();
> > 
> > As the comment says "We don't want in_nmi() to return true, but we need to
> > notify RCU.". However, since kernel 5.8 the logic of how interrupts are handled
> > was modified and currently we have this (function "exc_int3"):
> > https://elixir.bootlin.com/linux/v5.8/source/arch/x86/kernel/traps.c#L630
> > 
> >     /*
> >      * idtentry_enter_user() uses static_branch_{,un}likely() and therefore
> >      * can trigger INT3, hence poke_int3_handler() must be done
> >      * before. If the entry came from kernel mode, then use nmi_enter()
> >      * because the INT3 could have been hit in any context including
> >      * NMI.
> >      */
> >     if (user_mode(regs)) {
> >         idtentry_enter_user(regs);
> >         instrumentation_begin();
> >         do_int3_user(regs);
> >         instrumentation_end();
> >         idtentry_exit_user(regs);
> >     } else {
> >         nmi_enter();
> >         instrumentation_begin();
> >         trace_hardirqs_off_finish();
> >         if (!do_int3(regs))
> >             die("int3", regs, 0);
> >         if (regs->flags & X86_EFLAGS_IF)
> >             trace_hardirqs_on_prepare();
> >         instrumentation_end();
> >         nmi_exit();
> >     }
> > 
> > The root of unlucky change comes from this commit:
> > 
> > https://github.com/torvalds/linux/commit/0d00449c7a28a1514595630735df383dec606812#diff-51ce909c2f65ed9cc668bc36cc3c18528541d8a10e84287874cd37a5918abae5
> > 
> > which later was modified by this commit:
> > 
> > https://github.com/torvalds/linux/commit/8edd7e37aed8b9df938a63f0b0259c70569ce3d2
> > 
> > and this is what we currently have in all kernels since 5.8. Essentially,
> > KRETPROBES are not working since these commits. We have the following logic:
> > 
> > asm_exc_int3() -> exc_int3():
> >                     |
> >     ----------------|
> >     |
> >     v
> > ...
> > nmi_enter();
> > ...
> > if (!do_int3(regs))
> >        |
> >   -----|
> >   |
> >   v
> > do_int3() -> kprobe_int3_handler():
> >                     |
> >     ----------------|
> >     |
> >     v
> > ...
> > if (!p->pre_handler || !p->pre_handler(p, regs))
> >                              |
> >     -------------------------|
> >     |
> >     v
> > ...
> > pre_handler_kretprobe():
> > ...
> >     if (unlikely(in_nmi())) {
> >         rp->nmissed++;
> >         return 0;
> >     }
> > 
> > Essentially, exc_int3() calls nmi_enter(), and pre_handler_kretprobe() before
> > invoking any registered kprobe verifies if it is not in NMI via in_nmi() call.
> > 
> > This bug was masked by OPTIMIZER which was turning most of the KPROBES to be
> > FTRACE so essentially if someone registered KRETPROBE, buggy code was not
> > invoked (FTRACE code was executed instead). However, the optimizer was changed
> > and can't optimize as many functions anymore (probably another bug which might
> > be worth to investigate) and this bug is more visible.
> > 
> > Thanks,
> > Adam
> > 
> > -- 
> > pi3 (pi3ki31ny) - pi3 (at) itsec pl
> > http://pi3.com.pl
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

-- 
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl