Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Russell King - ARM Linux]

On Sat, Feb 11, 2017 at 07:33:16AM +0900, Masami Hiramatsu wrote:
> On Fri, 10 Feb 2017 11:34:45 +0900
> Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> > Ah, in_nmi() means FIQ on arm :)
> > OK, but actually it is too late to check it in the enter of
> > trampoline_handler() since we don't know where is the real
> > return address at that point. So I'll check that in setup site
> > - kretprobe_pre_handler().
> 
> Hmm, pre_handler_kretprobe() already checked in_nmi().
> So, I think this will no problem on FIQ too.

I don't blame you for missing that - the tracing and probes code is (at
least to me) quite a maze of code.

>From what I can tell, you're right - pre_handler_kretprobe() checks
in_nmi() early on, which prevents arch_prepare_kretprobe() (which
replaces regs->ARM_lr with the trampoline address) being run.  Hence,
the trampoline should not be run if we were entered in FIQ mode.

However, looking at kprobe_handler(), I'm much less convinced.  This is
called as a result of hitting a probe instruction via
kprobe_trap_handler().

Now, if we have two kprobes, one in non-FIQ context and one in FIQ
context, and the non-FIQ context one is hit, we set the current kprobe:

                } else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
                        /* Probe hit and conditional execution check ok. */
                        set_current_kprobe(p);
                        kcb->kprobe_status = KPROBE_HIT_ACTIVE;

and call the pre-handler (which succeeds.)  If we then take a FIQ and
hit a kprobe in a function called from FIQ, we will re-enter this
function.

In this case, "cur" will be the non-FIQ kprobe, and "p" will be the FIQ
kprobe.  It looks to me like we will single-step over the kprobe, and
resume.  However, it will modify the kprobe_status to KPROBE_REENTER,
which may not be desirable.

However, there does seem to be a hole.  Let's say that we have a similar
scenario, except that the FIQ is well-timed to happen:

                        if (!p->pre_handler || !p->pre_handler(p, regs)) {
                                kcb->kprobe_status = KPROBE_HIT_SS;
/* HERE */
                                singlestep(p, regs, kcb);
                                if (p->post_handler) {
                                        kcb->kprobe_status = KPROBE_HIT_SSDONE;

In that case:

                        /* Kprobe is pending, so we're recursing. */
                        switch (kcb->kprobe_status) {
                        case KPROBE_HIT_ACTIVE:
                        case KPROBE_HIT_SSDONE:
...
                        default:
                                /* impossible cases */
                                BUG();

becomes not such an "impossible case", so the kernel is likely to
explode.

This doesn't look good to me, and the pre-handler does nothing to
prevent this, so I still think we need some higher level protection in
kprobe_handler() against being entered in FIQ context - not only to
prevent that BUG() but also to prevent the kprobe status being changed
to "re-enter".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.