linux-kernel - Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe

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]

Date:   Sat, 16 Sep 2017 17:03:55 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
        Anton Blanchard <anton@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe

On 2017/09/15 05:38PM, Oleg Nesterov wrote:
> Hi Naveen,

Hi Oleg,

> 
> I forgot almost everything about this code, but at first glance this patch
> needs more comments and the changelog should be updated, at least. And in
> any case this needs more changes iirc.

Sure, thanks for the review!

> 
> On 09/14, Naveen N. Rao wrote:
> >
> > If we try to install a uprobe on a breakpoint instruction, we register the
> > probe, but refuse to install it. In this case, when the breakpoint hits, we
> > incorrectly assume that the probe hit and end up looping.
> 
> This looks confusing to me...
> 
> If we try to install a uprobe on a breakpoint instruction, uprobe_register()
> should fail and remove uprobe. The task which hits this uprobe will loop
> until this uprobe goes away, this is fine.
> 
> But there is another case and probably this is what you mean. uprobe_register()
> can do nothing except insert_uprobe() if nobody mmaps this binary, and after that
> uprobe_mmap()->prepare_uprobe() can fail if the original isns is int3. In this
> case this !UPROBE_COPY_INSN uprobe won't go away, and the task can loop until
> it is killed or uprobe_unregister().
> 
> Right?

You're right -- I should have elaborated.

> 
> 
> Now. The real fix should kill UPROBE_COPY_INSN altogether and simply move
> prepare_uprobe() from install_breakpoint() into __uprobe_register(), before
> it does register_for_each_vma().
> 
> The only problem is that read_mapping_page() needs "struct file *" and nobody
> confirmed that read_mapping_page(data => NULL) is fine on all filesystems.
> I think it should be fine though, and perhaps we should finally do this change.

Sure. I will take a stab at this after these fixes.

> 
> 
> until then we can probably make the things a bit better, but
> 
> > Fix this by checking that the trap was actually installed in
> > find_active_uprobe().
> > 
> > Reported-by: Anton Blanchard <anton@...ba.org>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com>
> > ---
> >  kernel/events/uprobes.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index e14eb0a6e4f3..599078e6a092 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1752,6 +1752,13 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >  			uprobe = find_uprobe(inode, offset);
> >  		}
> >  
> > +		/* Ensure that the breakpoint was actually installed */
> > +		if (uprobe) {
> > +			smp_rmb(); /* pairs with wmb() in prepare_uprobe() */
> > +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> > +				uprobe = NULL;
> > +		}
> 
> I need to recall how this code works... but if we add this change, shouldn't
> we remove another similar UPROBE_COPY_INSN check in handle_swbp() and add more
> comments?

Yes, you're right. The check in handle_swbp() won't be needed anymore. I 
will make that change and re-spin.

Thanks,
Naveen