linux-kernel - Re: [PATCH v3 2/2] powerpc: Uprobes port to powerpc

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:	Fri, 17 Aug 2012 17:00:31 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Ananth N Mavinakayanahalli <ananth@...ibm.com>
Cc:	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	linuxppc-dev@...ts.ozlabs.org, lkml <linux-kernel@...r.kernel.org>,
	Paul Mackerras <paulus@...ba.org>,
	Anton Blanchard <anton@...ba.org>, michael@...erman.id.au,
	Ingo Molnar <mingo@...e.hu>, peterz@...radead.org,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Subject: Re: [PATCH v3 2/2] powerpc: Uprobes port to powerpc

On 08/17, Ananth N Mavinakayanahalli wrote:
>
> On Thu, Aug 16, 2012 at 05:21:12PM +0200, Oleg Nesterov wrote:
>
> > Hmm, I am not sure. is_swbp_insn(insn), as it is used in the arch agnostic
> > code, should only return true if insn == UPROBE_SWBP_INSN (just in case,
> > this logic needs more fixes but this is offtopic).
>
> I think it does...
>
> > If powerpc has another insn(s) which can trigger powerpc's do_int3()
> > counterpart, they should be rejected by arch_uprobe_analyze_insn().
> > I think.
>
> The insn that gets passed to arch_uprobe_analyze_insn() is copy_insn()'s
> version, which is the file copy of the instruction.

Yes, exactly. And we are going to single-step this saved uprobe->arch.insn,
even if gdb/whatever replaces the original insn later or already replaced.

So arch_uprobe_analyze_insn() should reject the "unsafe" instructions which
we can't step over safely.

> We should also take
> care of the in-memory copy, in case gdb had inserted a breakpoint at the
> same location, right?

gdb (or even the application itself) and uprobes can obviously confuse
each other, in many ways, and we can do nothing at least currently.
Just we should ensure that the kernel can't crash/hang/etc.

> Updating is_swbp_insn() per-arch where needed will
> take care of both the cases, 'cos it gets called before
> arch_analyze_uprobe_insn() too.

For example. set_swbp()->is_swbp_insn() == T means that (for example)
uprobe_register() and uprobe_mmap() raced with each other and there is
no need for set_swbp().

However, find_active_uprobe()->is_swbp_at_addr()->is_swbp_insn() is
different, "true" confirms that this insn has triggered do_int3() and
thus we need send_sig(SIGTRAP) (just in case, this is not strictly
correct too but offtopic again).

We definitely need more changes/fixes/improvements in this area. And
perhaps powerpc requires more changes in the arch-neutral code, I dunno.
In particular, I think is_swbp_insn() should have a single caller,
is_swbp_at_addr(), and this caller should always play with current->mm.
And many, many other changes in the long term.

So far I think that, if powerpc really needs to change is_swbp_insn(),
it would be better to make another patch and discuss this change.
But of course I can't judge.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/