linux-kernel - Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails

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: <20150518130057.GA25260@dhcp-128-1.nay.redhat.com>
Date:	Mon, 18 May 2015 21:00:57 +0800
From:	Minfei Huang <mhuang@...hat.com>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Josh Poimboeuf <jpoimboe@...hat.com>,
	Minfei Huang <mnfhuang@...il.com>, mbenes@...e.cz,
	sjenning@...hat.com, jkosina@...e.cz, vojtech@...e.cz,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] livepatch: Prevent to apply the patch once coming
 module notifier fails

On 05/18/15 at 02:08pm, Petr Mladek wrote:
> On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
> > >  }
> > >  EXPORT_SYMBOL_GPL(klp_register_patch);
> > >  
> > > -static void klp_module_notify_coming(struct klp_patch *patch,
> > > +static int klp_module_notify_coming(struct klp_patch *patch,
> > >  				     struct klp_object *obj)
> > >  {
> > >  	struct module *pmod = patch->mod;
> > > @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> > >  	int ret;
> > >  
> > >  	ret = klp_init_object_loaded(patch, obj);
> > > -	if (ret)
> > > -		goto err;
> > > +	if (ret) {
> > > +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> > > +				pmod->name, ret);
> > > +		goto out;
> > > +	}
> > 
> > Can you change it to:
> > 
> > "failed to initialize the patch '%s' for module '%s' (%d)\n" ?
> > 
> > That would make it more consistent with the other error message and
> > identify the failing module.
> > 
> > Also, the indentation should be fixed on the second pr_warn() line.
> > 
> > >  
> > >  	if (patch->state == KLP_DISABLED)
> > > -		return;
> > > +		goto out;
> > >  
> > >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> > >  		  pmod->name, mod->name);
> > >  
> > >  	ret = klp_enable_object(obj);
> > > -	if (!ret)
> > > -		return;
> > > -
> > > -err:
> > > -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > -		pmod->name, mod->name, ret);
> > > +	if (ret)
> > > +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > +				pmod->name, mod->name, ret);
> > 
> > Bad indentation here too.
> > 
> > > @@ -930,6 +932,7 @@ disabled:
> > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  			     void *data)
> > >  {
> > > +	int ret;
> > >  	struct module *mod = data;
> > >  	struct klp_patch *patch;
> > >  	struct klp_object *obj;
> > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  
> > >  			if (action == MODULE_STATE_COMING) {
> > >  				obj->mod = mod;
> > > -				klp_module_notify_coming(patch, obj);
> > > +				ret = klp_module_notify_coming(patch, obj);
> > > +				if (ret) {
> > > +					obj->mod = NULL;
> > > +					pr_warn("patch '%s' is dead, remove it "
> > > +						"or re-install the module '%s'\n",
> > > +						patch->mod->name, obj->name);
> > > +				}
> > 
> > The patch isn't necessarily dead, since it might also include previously
> > enabled changes for vmlinux or other modules.  It can actually be a
> > dangerous condition if there's a mismatch between old code in the module
> > and new code elsewhere.  How about something like:
> > 
> > "patch '%s' is in an inconsistent state!\n"
> 
> It must not be dangerous, otherwise the patch could not get applied
> immediately.

But kernel is in dangerous situation that the patch may corrupt it
later. So it is appropriate to notify the user.

> 
> I would omit this message completely. It would just duplicate the
> warning printed by klp_module_notify_coming().
> 

This error message aims to tell this fact that this patch is in an
inconsistent state. If someone do not notify this error, it is fine,
because the inconsistent patch does not have change to be applied to the
kernel.

> 
> > Also, there's no need to split up the string literal into two lines.
> > It's ok for a line to have more than 80 columns in that case.
> 
> I suggest to run ./scripts/chechpatch.pl before you send any patch.
> It would catch the indentation problems, split of the string, ...

I have used the script checkpatch.pl to verify the patch. And it passed
for the checking. About the indentation problems, it may be caused by
the sepcified vimrc file.

Thanks
Minfei

> 
> Best Regards,
> Petr
--
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/