linux-kernel - [BUG] Incorrect ordering of klp and ftrace module notifier callbacks

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-next>] [day] [month] [year] [list]

Message-ID: <20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com>
Date:	Thu, 28 Jan 2016 15:40:33 -0500
From:	Jessica Yu <jeyu@...hat.com>
To:	Josh Poimboeuf <jpoimboe@...hat.com>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>,
	Miroslav Benes <mbenes@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>
Cc:	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [BUG] Incorrect ordering of klp and ftrace module notifier callbacks

Hi all,

I recently found that livepatch is broken for patches to modules on the latest
for-next branch [1]. In its current state, livepatch is able to patch a module
if the module is *already* loaded, but will fail if the target module loads
after the patch module loads. The patched, new functions are not getting
called, and the old functions are running instead.

I ran a git bisect with the last good commit as -
[b56b36e] livepatch: Cleanup module page permission changes
and the first bad commit identified was -
[5156dca] ftrace: Fix the race between ftrace and insmod

In short, that commit moves the call to ftrace_module_enable() out of
ftrace_module_init() to the ftrace_module_notify() callback, and is called when
the module state is COMING. The problem is, livepatch also has a module
notifier callback that enables a patch for COMING modules, but makes the
assumption that ftrace_module_enable() has already been called. So ordering is
important for livepatch to get this right, but with the current notifier
priorities, we get the ordering wrong.

So we currently get the following callback ordering when a target module loads:
1) klp_module_notify() [priority: INT_MIN+1, so this runs first] -
Sees target module is COMING, tries to enable the corresponding patch
(but ftrace_module_enable() hasn't been called yet)
2) ftrace_module_notify() [priority: INT_MIN, runs last] -
Sees module is COMING, calls ftrace_module_enable(), but it's too late for
livepatch

And the following ordering when the target module unloads:
1) klp_module_notify() [priority: INT_MIN+1] -
Sees target module GOING, disables patch
2) ftrace_module_notify [priority: INT_MIN] -
Sees module GOING, calls ftrace_release_mod()

The unload ordering is correct, but for the load case the calls need to be
flipped around (ftrace first).

So it doesn't look like a simple priority tweak will work, since we want a
ftrace->klp->klp->ftrace ordering from module load->module unload. Perhaps we
can split the ftrace and klp notifiers into separate coming and going handlers
with the appropriate priorities set to enforce correct ordering (not terribly
pretty, but it works). Is there a better solution?

[1] git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-next

Thanks,
Jessica