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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170210232113.GP27312@n2100.armlinux.org.uk>
Date:   Fri, 10 Feb 2017 23:21:13 +0000
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Masami Hiramatsu <mhiramat@...nel.org>
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 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.

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.