linux-kernel - Re: [PATCH v4 1/3] livepatch: add (un)patch 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170829154902.sw7r4k3ofo7hv4ev@treble>
Date:   Tue, 29 Aug 2017 10:49:02 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Joe Lawrence <joe.lawrence@...hat.com>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jessica Yu <jeyu@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Petr Mladek <pmladek@...e.com>,
        Chris J Arges <chris.j.arges@...onical.com>
Subject: Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> +Test 6
> +------
> +
> +Test a scenario where a vmlinux pre-patch callback returns a non-zero
> +status (ie, failure):
> +
> +- load target module
> +- load livepatch -ENODEV
> +- unload target module
> +
> +First load a target module:
> +
> +  % insmod samples/livepatch/livepatch-callbacks-mod.ko
> +  [   80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> +
> +Load the livepatch module, setting its 'pre_patch_ret' value to -19
> +(-ENODEV).  When its vmlinux pre-patch callback executed, this status
> +code will propagate back to the module-loading subsystem.  The result is
> +that the insmod command refuses to load the livepatch module:
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
> +  [   82.747326] livepatch: enabling patch 'livepatch_callbacks_demo'
> +  [   82.747743] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> +  [   82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> +  [   82.748237] livepatch: pre-patch callback failed for object 'vmlinux'
> +  [   82.748637] livepatch: failed to enable patch 'livepatch_callbacks_demo'
> +  [   82.749059] livepatch: 'livepatch_callbacks_demo': canceling transition, unpatching
> +  [   82.749060] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
> +  [   82.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> +  [   82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete
> +  [   82.765809] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
> +
> +  % rmmod samples/livepatch/livepatch-callbacks-mod.ko
> +  [   84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit

First off, this documentation is very nice because it clarifies all the
callback scenarios and edge cases.

The above situation still seems a little odd to me.  If I understand
correctly, the target module was never patched, and its pre_patch
callback was never called.  But its post_unpatch callback *was* called.
That doesn't seem right.

Maybe we should change the condition a little bit.  Currently it's:

  No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
  for a given klp_object if its pre-patch callback returned non-zero
  status.

I think that might have been my idea, but seeing the above case makes it
clear that it's not quite right.  Maybe it should instead be:

  No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
  for a given klp_object if the object failed to patch, due to a failed
  pre_patch callback or for any other reason.

  If the object did successfully patch, but the patch transition never
  started for some reason (e.g., if another object failed to patch),
  only the post-unpatch callback will be called.

So then, instead of tracking whether the pre-patch callback succeeded,
we just need to track whether the object was patched (which we already
do, with obj->patched).

What do you think?

-- 
Josh