linux-kernel - Re: [PATCH 1/6] livepatch v5: avoid race when checking for state of the patch

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]

Date:	Wed, 10 Dec 2014 11:11:47 +0100
From:	Petr Mladek <pmladek@...e.cz>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Miroslav Benes <mbenes@...e.cz>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Christoph Hellwig <hch@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] livepatch v5: avoid race when checking for state of
 the patch

On Tue 2014-12-09 12:32:49, Josh Poimboeuf wrote:
> On Tue, Dec 09, 2014 at 07:05:02PM +0100, Petr Mladek wrote:
> > klp_patch_enable() and klp_patch_disable() should check the current state
> > of the patch under the klp_lock. Otherwise, it might detect that the operation
> > is valid but the situation might change before it takes the lock.
> 
> Hi Petr,
> 
> Thanks for the patches.
> 
> I don't think this patch is necessary.  klp_is_enabled() doesn't check
> the state of the patch.  It checks the initialization state of the core
> module (klp_root_kobj), which can only be set in klp_init().  It's not
> protected by the lock, so I don't see the point of this patch.

Ah, I have misread the name and expected that it checked whether
the patch was enabled or disabled. The original code is OK then.

Well, Jiri Kosina pointed out that the check did not make much sense.
klp_is_enabled() could not be called if the livepatch module is not
loaded. And the later check for klp_patch_is_registered() is enough
to check whether the klp_enable_patch()/klp_disable_patch() calls
are allowed or not.

So, I suggest to remove the checks at all.

Best Regards,
Petr


> > 
> > Signed-off-by: Petr Mladek <pmladek@...e.cz>
> > ---
> >  kernel/livepatch/core.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index d6d0f50e81f8..b848069e44cc 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -425,11 +425,13 @@ int klp_disable_patch(struct klp_patch *patch)
> >  {
> >  	int ret;
> >  
> > -	if (!klp_is_enabled())
> > -		return -ENODEV;
> > -
> >  	mutex_lock(&klp_mutex);
> >  
> > +	if (!klp_is_enabled()) {
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> >  	if (!klp_patch_is_registered(patch)) {
> >  		ret = -EINVAL;
> >  		goto err;
> > @@ -489,11 +491,13 @@ int klp_enable_patch(struct klp_patch *patch)
> >  {
> >  	int ret;
> >  
> > -	if (!klp_is_enabled())
> > -		return -ENODEV;
> > -
> >  	mutex_lock(&klp_mutex);
> >  
> > +	if (!klp_is_enabled()) {
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> >  	if (!klp_patch_is_registered(patch)) {
> >  		ret = -EINVAL;
> >  		goto err;
> > -- 
> > 1.8.5.2
> > 
> 
> -- 
> Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/