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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 11 Feb 2017 18:21:48 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H . Peter Anvin" <hpa@...or.com>, Jon Medhurst <tixy@...aro.org>,
        Wang Nan <wangnan0@...wei.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        "David A . Long" <dave.long@...aro.org>,
        Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
Subject: Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible
 deadlock case in kretprobe

On Fri, 10 Feb 2017 23:21:13 +0000
Russell King - ARM Linux <linux@...linux.org.uk> 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@...nel.org> 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@...nel.org>