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
| ||
|
Message-Id: <20170916113355.orh5cswy3qehltfc@naverao1-tp.localdomain> 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
Powered by blists - more mailing lists