linux-kernel - Re: [PATCH 0/2] kprobes: improve error handling when arming/disarming kprobes

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:   Thu, 5 Oct 2017 15:09:53 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>, Jessica Yu <jeyu@...nel.org>
Cc:     Petr Mladek <pmladek@...e.com>,
        Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        Ingo Molnar <mingo@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] kprobes: improve error handling when
 arming/disarming kprobes

On Wed, 4 Oct 2017 15:44:13 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> Masami,
> 
> Can you review these patches?

Yes, of course :)

> 
> -- Steve
> 
> 
> On Wed,  4 Oct 2017 21:14:12 +0200
> Jessica Yu <jeyu@...nel.org> wrote:
> 
> > Hi,
> > 
> > This patchset attempts to improve error handling when arming or disarming
> > ftrace-based kprobes. The current behavior is to simply WARN when ftrace
> > (un-)registration fails, without propagating the error code. This can lead
> > to confusing situations where, for example, register_kprobe()/enable_kprobe()
> > would return 0 indicating success even if arming via ftrace had failed. In
> > this scenario we'd end up with a non-functioning kprobe even though kprobe
> > registration (or enablement) returned success. In this patchset, we take
> > errors from ftrace into account and propagate the error when we cannot arm
> > or disarm a kprobe.
> > 
> > Below is an example that illustrates the problem using livepatch and
> > systemtap (which uses kprobes underneath). Both livepatch and kprobes use
> > ftrace ops with the IPMODIFY flag set, so registration at the same
> > function entry is limited to only one ftrace user. 
> > 
> > Before
> > ------
> > # modprobe livepatch-sample 	# patches cmdline_proc_show, ftrace ops has IPMODIFY set
> > # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
> > 
> >    .. (nothing prints after reading /proc/cmdline) ..
> > 
> > The systemtap handler doesn't execute due to a kprobe arming failure caused
> > by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
> > indication of error from systemtap (because register_kprobe() returned
> > success) unless the user inspects dmesg.

Ah, right. It should be handled.

> > 
> > After
> > -----
> > # modprobe livepatch-sample 
> > # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
> > WARNING: probe kernel.function("cmdline_proc_show@...me/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)
> > 
> > Although the systemtap handler doesn't execute (as it shouldn't), the
> > ftrace error is propagated and now systemtap prints a visible error message
> > stating that (kprobe) registration had failed (because register_kprobe()
> > returned an error), along with the propagated error code.

Would you have any test-case for this livepatch-kprobes combination?

Thank you,

> > 
> > This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
> > back in 2015, which improved kprobes error handling, found here:
> > 
> >    https://lkml.org/lkml/2015/2/26/452
> > 
> > However, further work on this had been paused since then and the patches
> > were not upstreamed.
> > 
> > This patchset has been lightly sanity-tested (on linux-next) with kprobes,
> > kretprobes, jprobes, and optimized kprobes. It passes the kprobes smoke
> > test, but more testing is greatly appreciated.
> > 
> > Thanks,
> > 
> > Jessica
> > 
> > ---
> > Jessica Yu (2):
> >   kprobes: propagate error from arm_kprobe_ftrace()
> >   kprobes: propagate error from disarm_kprobe_ftrace()
> > 
> >  kernel/kprobes.c | 163 ++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 112 insertions(+), 51 deletions(-)
> > 
> 


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