linux-kernel - Re: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.

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: <20180411123214.mfpkbrirze32phrb@treble>
Date:   Wed, 11 Apr 2018 07:32:14 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Petr Mladek <pmladek@...e.com>, Jiri Kosina <jikos@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Jessica Yu <jeyu@...nel.org>,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 05/10] livepatch: Support separate list for replaced
 patches.

On Wed, Apr 11, 2018 at 10:07:31AM +0200, Miroslav Benes wrote:
> > > I was confused by wording "in the middle". It suggested that there
> > > might had been enabled patches on the top and the bottom of the stack
> > > and some disabled patches in between at the same time (or vice versa).
> > > This was not true.
> > 
> > That *was* what I meant.  Consider the following sequence of events:
> > 
> > - Register patch 1
> > - Enable patch 1
> > - Register patch 2
> > - Enable patch 2
> > - Disable patch 2
> > - Register patch 3
> > - Enable patch 3
> > 
> > Notice that patch 2 (in the middle) is disabled, whereas patch 1 (on the
> > bottom) and patch 3 (on the top) are enabled.
> 
> This should not be possible at all.
> 
> __klp_enable_patch:
> 
>         if (patch->list.prev != &klp_patches &&
>             !list_prev_entry(patch, list)->enabled)
>                 return -EBUSY;
> 
> When patch 3 is enabled, list_prev_entry() returns patch 2 and its 
> ->enabled is false.

Hm, you're right.  I'm not sure how I got that idea...

I still agree with my original conclusion that enforcing stack order no
longer makes sense though.

> > > Another possibility would be to get rid of the enable/disable states.
> > > I mean that the patch will be automatically enabled during
> > > registration and removed during unregistration.
> > 
> > I don't see how disabling during unregistration would be possible, since
> > the unregister is called from the patch module exit function, which
> > can only be called *after* the patch is disabled.
> > 
> > However, we could unregister immediately after disabling (i.e., in
> > enabled_store context).
> 
> I think this is what Petr meant. So there would be nothing in the patch 
> module exit function. Well, not exactly. We'd need to remove sysfs dir and 
> maybe something more.

Sounds good to me, though aren't the livepatch sysfs entries removed by
klp during unregister?

> > > The question is what is acceptable to others
> > 
> > If there are any objections, this is their chance to speak up :-)
> > 
> > > and if it needs to be done as part of this patch set.
> > 
> > Maybe so, for at least a few reasons:
> > 
> > - This patch set makes the 'stack' obsolete, so it makes sense to remove
> >   the 'stack' with it.
> 
> Not necessarily. I like Petr's rebase explanation here.

I'm not sure what you mean.  IIRC, his rebase explanation referred to
how we handle 'replace' patches, for which there is no stacking (as I
meant the term: enforcement of stack order for registration and
enablement).

-- 
Josh