linux-kernel - Re: PATCH v6 6/6] livepatch: Add atomic replace

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: <20180126090901.z36n2tn67b2nz75m@pathway.suse.cz>
Date:   Fri, 26 Jan 2018 10:09:01 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Jason Baron <jbaron@...mai.com>
Cc:     jpoimboe@...hat.com, jikos@...nel.org, mbenes@...e.cz,
        jeyu@...nel.org, Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On Thu 2018-01-25 23:27:57, Jason Baron wrote:
> On 01/25/2018 11:02 AM, Petr Mladek wrote:
> > From: Jason Baron <jbaron@...mai.com>
> > A better solution would be to create cumulative patch and say that
> > it replaces all older ones.
> > 
> > Signed-off-by: Jason Baron <jbaron@...mai.com>
> > [pmladek@...e.com: Split, reuse existing code, simplified]
> 
> Hi Petr,
> 
> Thanks for cleaning this up - it looks good.

Uff, I feel relief :-)

> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 6ad3195d995a..c606b291dfcd 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > +/*
> > + * This function removes replaced patches from both func_stack
> > + * and klp_patches stack.
> > + *
> > + * We could be pretty aggressive here. It is called in situation
> > + * when these structures are not longer accessible. All functions
> > + * are redirected using the klp_transition_patch. They use either
> > + * a new code or they in the original code because of the special
> > + * nop function patches.
> > + */
> > +void klp_throw_away_replaced_patches(struct klp_patch *new_patch,
> > +				     bool keep_module)
> > +{
> > +	struct klp_patch *old_patch, *tmp_patch;
> > +
> > +	list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) {
> > +		if (old_patch == new_patch)
> > +			return;
> > +
> > +		klp_unpatch_objects(old_patch, KLP_FUNC_ANY);
> > +		old_patch->enabled = false;
> > +
> > +		/*
> > +		 * Replaced patches could not get re-enabled to keep
> > +		 * the code sane.
> > +		 */
> > +		list_del_init(&old_patch->list);
> 
> I'm wondering if this should be:
> 
> list_move(&old_patch->list, &klp_replaced_patches);

Yes, great catch!

The list_del() comes from one iteration where I got rid of the extra
list. I though that it might be enough to check
patch->kobj.state_initialized. But then I realized that this
kobject state was modified outside klp_mutex.

To be honest, I did not only minimal testing with my changes.
Mirek promised to port a battery of his kGraft-based tests and
run it.

Thanks a lot for review.

Best Regards,
Petr