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

[Masami Hiramatsu]

On Fri, 10 Feb 2017 23:21:13 +0000
Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx> wrote:

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

Right.

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

OK, this one should be a bug on arm implementation.
On x86, we also check status == KPROBE_HIT_SS too, see reenter_kprobe()
at arch/x86/kernel/kprobes/core.c. (see commit 6a5022a56)
It seems same issue on arm64. I'll fix that.

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

What would you mean higher level?

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>