linux-kernel - Re: [PATCH -tip v4 24/27] bpf: error-inject: kprobes: Clear current

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:   Mon, 4 Jun 2018 18:08:44 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        Arnd Bergmann <arnd@...db.de>,
        Alexei Starovoitov <ast@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "H . Peter Anvin" <hpa@...or.com>, Josef Bacik <jbacik@...com>,
        James Hogan <jhogan@...nel.org>,
        Laura Abbott <labbott@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tony Luck <tony.luck@...el.com>,
        Vineet Gupta <vgupta@...opsys.com>,
        Will Deacon <will.deacon@....com>, x86@...nel.org
Subject: Re: [PATCH -tip v4 24/27] bpf: error-inject: kprobes: Clear
 current_kprobe and enable preempt in kprobe

On Sat, 02 Jun 2018 17:28:05 +0530
"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:

> Masami Hiramatsu wrote:
> > On Thu, 31 May 2018 16:25:38 +0530
> > "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:
> > 
> >> Masami Hiramatsu wrote:
> >> > Clear current_kprobe and enable preemption in kprobe
> >> > even if pre_handler returns !0.
> >> > 
> >> > This simplifies function override using kprobes.
> >> > 
> >> > Jprobe used to require to keep the preemption disabled and
> >> > keep current_kprobe until it returned to original function
> >> > entry. For this reason kprobe_int3_handler() and similar
> >> > arch dependent kprobe handers checks pre_handler result
> >> > and exit without enabling preemption if the result is !0.
> >> > 
> >> > After removing the jprobe, Kprobes does not need to
> >> > keep preempt disabled even if user handler returns !0
> >> > anymore.
> >> 
> >> I think the reason jprobes did it that way is to address architecture 
> >> specific requirements when changing a function. So, without that 
> >> infrastructure, I am not sure if we will be able to claim support for 
> >> over-riding functions with kprobes. I am not sure if we want to claim 
> >> that, but this is something we need to be clear on.
> > 
> > Really? as far as I can see, there seems no such architecture.
> > The keeping preempt disabled is corresponding to keeping current_kprobe
> > since the current_kprobe is per-cpu.
> 
> Right, and the reason for not resetting current_kprobe after kprobe 
> handling is done is primarily for jprobes. 
> 
> > This means if it is preempted
> > before hitting break_handler and changed cpu core, we missed to
> > handle current_kprobe and goes to panic. But if we don't need
> > such "break back" (removing break_handler), we don't need to
> > keep current_kprobe (because it is not handled afterwards).
> 
> Agreed.
> 
> > 
> > Anyway, changing function execution path is a "one-way" change.
> 
> This is the problem. With jprobes, over-riding a function was not a 
> "one-way" change because it involves more than just changing the [n]ip.  
> That is the reason we had setjmp/longjmp (aka break_handler).
> 
> > We don't have a chance to fixup that disabled preemption and current_kprobe
> > after returning to the new function. So current error-inject clears
> > current_kprobe and enable preemption before returning !0 from its
> > kprobe pre_handler.
> > 
> > This is just moving such needless operation from user-pre_handler to
> > kprobes itself. 
> > 
> >> For powerpc, the current function override in error-inject works fine 
> >> since the new function does nothing. But, if anyone wants to do more 
> >> work in the replacement function, it won't work with the current 
> >> approach.
> > 
> > If you are considering about TOC change etc. yes, it depends on
> > the archtecture. As far as I know IA64 and powerpc will not allow
> > to support changing execution path without special care.
> > Other "flat and simple" function call architectures like x86, arm
> > can change execution path without special care. 
> 
> Yes, that's the concern. As I stated earlier, the only user seems to be 
> error-injection where this is not a concern. I wanted this to be made 
> clear.
> 
> I've since noticed that you are updating Documentation/kprobes.txt to 
> make this clear in patch 24/27 in this series. So, I'm ok with the 
> changes in this series.

I see your concern. Yeah, that's why I added [24/27] for clearly stating it.

Thanks,


-- 
Masami Hiramatsu <mhiramat@...nel.org>