linux-kernel - Re: [PATCH v3] 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] [day] [month] [year] [list]

Message-ID: <20170822091711.GC30286@pathway.suse.cz>
Date:   Tue, 22 Aug 2017 11:17:11 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Joe Lawrence <joe.lawrence@...hat.com>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Chris J Arges <chris.j.arges@...onical.com>
Subject: Re: [PATCH v3] livepatch: add (un)patch callbacks

On Mon 2017-08-21 17:18:58, Joe Lawrence wrote:
> On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> > On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > > Provide livepatch modules a klp_object (un)patching notification
> > > mechanism.  Pre and post-(un)patch callbacks allow livepatch modules to
> > > setup or synchronize changes that would be difficult to support in only
> > > patched-or-unpatched code contexts.
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..ddb23e18a357 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > >  				goto err;
> > >  			}
> > >  
> > > +			klp_post_patch_callback(obj);
> > 
> > This should be called only if (patch != klp_transition_patch).
> > Otherwise, it would be called too early.
> 
> Can you elaborate a bit on this scenario?  When would the transition
> patch (as I understand it, a livepatch not quite fully (un)patched) hit
> the module coming/going notifier?  Is it possible to load or unload a
> module like this?  I'd like to add this scenario to my test script if
> possible.

if (patch == klp_transition_patch) then the transition for this
patch is still running. klp_post_patch_callback() should and is
called at the end of the transition, see klp_complete_transition().
Note that klp_complete_transition() will see the object/module
loaded because obj->mod is set in klp_module_coming().

The (up)patch transition might take a while. It might be even
infinite if a process is blocked in a patched function. klp_mutex
is available most of the time. Therefore it is perfectly fine to
load/remove modules into/from the system and run their
klp_module_coming()/going() callbacks when a livepatch
patch transition is running.


> > > +
> > >  			break;
> > >  		}
> > >  	}
> > > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > >  			if (patch->enabled || patch == klp_transition_patch) {
> > >  				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > >  					  patch->mod->name, obj->mod->name);
> > > +
> > > +				klp_pre_unpatch_callback(obj);
> > 
> > Also the pre_unpatch() callback should be called only
> > if (patch != klp_transition_patch). Otherwise, it should have
> > already been called. It is not the current case but see below.
> 
> Ditto.

This is related to the other comment where I and Josh suggested
to call klp_pre_unpatch_callback() in __klp_disable_patch()
before the transition started. It means that the callback
has already been called for klp_transition_patch.

Best Regards,
Petr